Martin Peřina has posted comments on this change. Change subject: uutils: added new cli parser ......................................................................
Patch Set 8: (17 comments) Since this is going to be used in all extensions that needs command line parsing and also in engine, then we really need proper JavaDoc for API https://gerrit.ovirt.org/#/c/40157/8/backend/manager/modules/uutils/src/main/java/org/ovirt/engine/core/uutils/cli/parser/ParametersParser.java File backend/manager/modules/uutils/src/main/java/org/ovirt/engine/core/uutils/cli/parser/ParametersParser.java: Line 19: import java.util.TreeSet; Line 20: import java.util.regex.Matcher; Line 21: import java.util.regex.Pattern; Line 22: Line 23: public class ParametersParser { I would prefer ArgumentParser as it describes better that is parses command line arguments Line 24: Line 25: /** Line 26: * Key under which are stored other arguments in map <code>arguments</code>. Line 27: * Use it to obtain rest of arguments, which was not parsed by parser. Line 33: */ Line 34: public static final String PARAMETER_KEY_ERRORS = "__error__"; Line 35: Line 36: private static final String LONG_PREFIX = "--"; Line 37: private static final Properties defaultProperties = loadProperties(ParametersParser.class, "defaults.properties"); Please use: private static final Properties defaultProperties = loadProperties( ParametersParser.class.getResourceAsStream("defaults.properties")); and remove unnecessary loadProperties(Class, String) Line 38: Line 39: private Properties properties; Line 40: private String prefix; Line 41: private Map<String, ParserArgument> arguments = new HashMap<>(); Line 40: private String prefix; Line 41: private Map<String, ParserArgument> arguments = new HashMap<>(); Line 42: private Set<String> mandatory = new HashSet<>(); Line 43: Line 44: static enum ArgumentType { Please remove static keyword, it's the default for inner enum Line 45: has_argument, Line 46: optional_argument, Line 47: no_argument, Line 48: } Line 43: Line 44: static enum ArgumentType { Line 45: has_argument, Line 46: optional_argument, Line 47: no_argument, 1. Please use Java Coding Conventions, enum value names should be in upper case. 2. Also I would prefer this naming so it's really clear what each value means: enum ArgumentValue { MANDATORY, OPTIONAL, NONE } 3. This enum doesn't belong here, but it should be part of ParserArgument (if it's used only internally) or it should be standalone enum (in its own file) Line 48: } Line 49: Line 50: public ParametersParser(Properties properties, String prefix) { Line 51: this.properties = properties; Line 52: this.prefix = prefix; Line 53: parseProperties(); Line 54: } Line 55: Line 56: public ParametersParser(InputStream resource, String prefix) { Is this really necessary? IMO the caller is responsible for providing Properties instance, ParametersParser should not do that. Line 57: this(loadProperties(resource), prefix); Line 58: } Line 59: Line 60: public Map<String, Object> parse(String... args) { Line 70: String arg = args.get(0); Line 71: if(!arg.startsWith(LONG_PREFIX)) { Line 72: break; Line 73: } Line 74: arg = args.remove(0); Usually it's a bad behavior to modify Collection provided by caller. Wouldn't it be better to reimplement it without modification of args? Line 75: if(arg.equals(LONG_PREFIX)) { Line 76: break; Line 77: } Line 78: Line 89: if ( Line 90: value == null && Line 91: ( Line 92: parserArgument.getType() == ArgumentType.optional_argument || Line 93: parserArgument.getType() == ArgumentType.has_argument Wouldn't it be better to use: if (value == null && parserArgument.getType() != ArgumentValue.NONE) { Line 94: ) Line 95: ) { Line 96: if(args.size() > 0) { Line 97: value = args.get(0); Line 94: ) Line 95: ) { Line 96: if(args.size() > 0) { Line 97: value = args.get(0); Line 98: if (value.startsWith("--")) { Please use LONG_PREFIX constant Line 99: value = null; Line 100: } else { Line 101: args.remove(0); Line 102: } Line 141: ); Line 142: } Line 143: others.addAll(args); Line 144: argMap.put(PARAMETER_KEY_OTHER, others); Line 145: argMap.put(PARAMETER_KEY_ERRORS, errors); I would prefer to store errors in its own attribute and to provide getErrors() method Line 146: Line 147: return argMap; Line 148: } Line 149: Line 162: private void putValue(Map<String, Object> argMap, ParserArgument arg, Object value) { Line 163: if (!arg.isMultivalue()) { Line 164: argMap.put(arg.getName(), value); Line 165: } else { Line 166: Collection c = (Collection) argMap.get(arg.getName()); Please use List<?> instead of Collection Line 167: if (c == null) { Line 168: c = new ArrayList<>(); Line 169: argMap.put(arg.getName(), c); Line 170: } https://gerrit.ovirt.org/#/c/40157/8/backend/manager/modules/uutils/src/main/java/org/ovirt/engine/core/uutils/cli/parser/ParserArgument.java File backend/manager/modules/uutils/src/main/java/org/ovirt/engine/core/uutils/cli/parser/ParserArgument.java: Line 1: package org.ovirt.engine.core.uutils.cli.parser; Line 2: Line 3: import java.util.regex.Pattern; Line 4: Line 5: public class ParserArgument { I would prefer just Argument Line 6: Line 7: private String name; Line 8: private String help; Line 9: private String defaultValue; Line 14: private String metavar; Line 15: private boolean multivalue; Line 16: private String value; Line 17: Line 18: public ParserArgument() {} Please remove this empty constructor Line 19: Line 20: public String getName() { Line 21: return name; Line 22: } Line 104: } Line 105: if(obj == this) { Line 106: return true; Line 107: } Line 108: Please use this order of conditions as it's considered best practise: if (this == obj) { return true; } if (!(obj instanceof ParserArgument)) { return false; } Line 109: return this.name.equals(((ParserArgument) obj).getName()); Line 110: } Line 111: Line 112: @Override Line 105: if(obj == this) { Line 106: return true; Line 107: } Line 108: Line 109: return this.name.equals(((ParserArgument) obj).getName()); Please use: return Objects.equals(name, ((ParserArgument) obj).getName()); Line 110: } Line 111: Line 112: @Override Line 113: public int hashCode() { Line 110: } Line 111: Line 112: @Override Line 113: public int hashCode() { Line 114: return 37 * name.hashCode(); Please use: return Objects.hash(name); Line 115: } https://gerrit.ovirt.org/#/c/40157/8/backend/manager/modules/uutils/src/main/java/org/ovirt/engine/core/uutils/cli/parser/Util.java File backend/manager/modules/uutils/src/main/java/org/ovirt/engine/core/uutils/cli/parser/Util.java: Line 10: import java.util.HashMap; Line 11: import java.util.List; Line 12: import java.util.Map; Line 13: Line 14: public class Util { Please don't ever use such common name. I would prefer: ArgumentValueConverter ArgumentParserUtils Line 15: Line 16: private static final Map<Class<?>, Class<?>> typeBox = new HashMap<>(); Line 17: static { Line 18: typeBox.put(boolean.class, Boolean.class); Line 32: if (clazz.isPrimitive()) { Line 33: clazz = typeBox.get(clazz); Line 34: } Line 35: Line 36: if (v == null) { Condition is always true here! Line 37: if (clazz.equals(Collection.class)) { Line 38: List<Object> r = new ArrayList<>(); Line 39: for (String c : value.trim().split(" *, *")) { Line 40: if (!c.isEmpty()) { -- To view, visit https://gerrit.ovirt.org/40157 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I00042b669e19293641579582223e7ca40717132d Gerrit-PatchSet: 8 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ondra Machacek <omach...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Martin Peřina <mper...@redhat.com> Gerrit-Reviewer: Mooli Tayer <mta...@redhat.com> Gerrit-Reviewer: Ondra Machacek <omach...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: mooli tayer <mta...@redhat.com> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
