mooli tayer has posted comments on this change. Change subject: uutils: added new cli parser ......................................................................
Patch Set 8: (4 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 __extra__ or __positional__? I think tail (or positional) is a better name for a new user of this class to understand what get("X") means. it describes what king of other/extra args these are. 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) > multiple errors are good service to user, instead of executing again and di ok. 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); > you cannot put default first as it is important to know if the parameter wa right my mistake 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, > if argument is mandatory why not always take default? mandatory & no value & has def = def not mandatory & no value & has def = def this is simpler to test & more powerful since user can decide if he wants default regardless of type. am I missing something? if so documentation can be something like: # if arg not provided set to this value #arg.defmissing # if no value provided set to this value #arg.defnoval 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 = -- 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
