Alon Bar-Lev has posted comments on this change. Change subject: uutils: added new cli parser ......................................................................
Patch Set 8: (3 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? why not __extra__ or __positional__? is that important, we have constant... this is kinda meaningless. 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 multiple errors are good service to user, instead of executing again and discover yet another error. it is up to program to display the first only or all. 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 cannot put default first as it is important to know if the parameter was provided or not, example: multi value. Line 133: Line 134: List<String> mandatoryCopy = new ArrayList<String>(mandatory); Line 135: mandatoryCopy.removeAll(argMap.keySet()); Line 136: if(!mandatoryCopy.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
