mooli tayer has posted comments on this change. Change subject: uutils: added new cli parser ......................................................................
Patch Set 8: (5 comments) nit comments. 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 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 28: */ Line 29: public static final String PARAMETER_KEY_OTHER = "__other__"; why not name this tail? (PARAMETER_KEY_TAIL) and __tail__ Line 30: /** Line 31: * Key under which are stored exceptions during parsing in map <code>arguments</code>. Line 32: * Use it to obtain exception which happend during parsing. Line 33: */ Line 104: } Line 105: if (parserArgument.getType() == ArgumentType.has_argument && value == null) { Line 106: errors.add( Line 107: new IllegalArgumentException( Line 108: String.format("Value is required, but missing for argument '%1$s'", key) I tend to think returning only first error is easier and you might be wrong trying to parse multiple: example: (name & value mandatory) xxxx --namw mike --value person you will think * namw bad key * value missing actually only * --namw instead of --name Up to you anyway. Line 109: ) Line 110: ); Line 111: } Line 112: if (value == null) { Line 128: } Line 129: putValue(argMap, parserArgument, convertedValue); Line 130: } Line 131: } Line 132: fillDefaults(argMap); put defaults first and then override? you can remove containsKey check that way. Also if you build mandatory during parsing you can avoid the copy. private Set<String> mandatory = new HashSet<>(); for(ParserArgument arg : arguments.values()) { if (arg.getDefaultValue() != null) { putValue } if (argument.isMandatory()) { mandatory.add(argument.getName()); } } while(!args.isEmpty()) ... Line 133: Line 134: List<String> mandatoryCopy = new ArrayList<String>(mandatory); Line 135: mandatoryCopy.removeAll(argMap.keySet()); Line 136: if(!mandatoryCopy.isEmpty()) { https://gerrit.ovirt.org/#/c/40157/8/backend/manager/modules/uutils/src/main/resources/org/ovirt/engine/core/uutils/cli/parser/defaults.properties File backend/manager/modules/uutils/src/main/resources/org/ovirt/engine/core/uutils/cli/parser/defaults.properties: Line 16: # argument can be specified more times, it will store list of values Line 17: arg.multivalue = false Line 18: # If argument has no value specified this value will be set Line 19: #arg.value = Line 20: # if argument is mandatory and not specified set it's value to arg.default, by default it's none, I'm dont understand. if it is NOT mandatory and not specified will it not be set to arg.default?! Line 21: # so if argument is mandatory and has no default value parsing will fail on IllegalArgumentException Line 22: #arg.default = Line 23: # will be print as first, should contain usage of program/module/action Line 24: help.usage = Line 17: arg.multivalue = false Line 18: # If argument has no value specified this value will be set Line 19: #arg.value = Line 20: # if argument is mandatory and not specified set it's value to arg.default, by default it's none, Line 21: # so if argument is mandatory and has no default value parsing will fail on IllegalArgumentException I think this comment belongs on arg.mandatory Line 22: #arg.default = Line 23: # will be print as first, should contain usage of program/module/action Line 24: help.usage = Line 25: # will be printed right after usage, should contain basic info about program/module/action -- 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
