Martin Peřina has posted comments on this change.

Change subject: uutils: added new cli parser
......................................................................


Patch Set 12:

(3 comments)

https://gerrit.ovirt.org/#/c/40157/12/backend/manager/modules/uutils/src/main/java/org/ovirt/engine/core/uutils/cli/parser/Argument.java
File 
backend/manager/modules/uutils/src/main/java/org/ovirt/engine/core/uutils/cli/parser/Argument.java:

Line 9:     private String help;
Line 10:     private String defaultValue;
Line 11:     private Type type;
Line 12:     private Pattern matcher;
Line 13:     private Class<?> convert;
> but this is a conversion from string to a type, it is a conversion process.
The attribute stores the type (class) which the string value should be 
converted to, so IMO that's why destinationType and valueType is better name 
for this
Line 14:     private boolean mandatory;
Line 15:     private String metavar;
Line 16:     private boolean multivalue;
Line 17:     private String value;


Line 15:     private String metavar;
Line 16:     private boolean multivalue;
Line 17:     private String value;
Line 18: 
Line 19:     enum Type {
> Can be HasArg per getopt_long or in properties has_arg, although the existi
Sorry, but HasArg IMO doesn't describe the enum any better than Type.
Line 20:         HAS_ARGUMENT,
Line 21:         OPTIONAL_ARGUMENT,
Line 22:         NO_ARGUMENT;
Line 23: 


https://gerrit.ovirt.org/#/c/40157/12/backend/manager/modules/uutils/src/main/java/org/ovirt/engine/core/uutils/cli/parser/ArgumentsParser.java
File 
backend/manager/modules/uutils/src/main/java/org/ovirt/engine/core/uutils/cli/parser/ArgumentsParser.java:

Line 152: 
Line 153:     /**
Line 154:      * Map which stores parsed converted arguments.
Line 155:      */
Line 156:     private Map<String, Argument> arguments = new HashMap<>();
> why?
It's good behavior and you don't look up through the whole file what's the 
default values
Line 157: 
Line 158:     /**
Line 159:      * Set of mandatory arguments to check if user specified all of 
them.
Line 160:      */


-- 
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: 12
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <omach...@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com>
Gerrit-Reviewer: Jenkins CI
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-HasComments: Yes
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to