Discussion:
Change in ovirt-engine[master]: uutils: added new cli parser
(too old to reply)
a***@ovirt.org
2015-04-22 19:00:43 UTC
Permalink
***@ovirt.org has posted comments on this change.

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


Patch Set 1:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-engine-3.5', 'ovirt-engine-3.4', 'ovirt-engine-3.3', 'ovirt-engine-3.5.2', 'ovirt-engine-3.4.0', 'ovirt-engine-3.3.4', 'ovirt-engine-3.3.3', 'ovirt-engine-3.3.2', 'ovirt-engine-3.3.1'])
--
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: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-HasComments: No
oVirt Jenkins CI Server
2015-04-22 19:00:51 UTC
Permalink
oVirt Jenkins CI Server has posted comments on this change.

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


Patch Set 1:

Build Started (1/2) -> http://jenkins.ovirt.org/job/ovirt-engine_master_unit-tests_gerrit/33056/
--
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: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
oVirt Jenkins CI Server
2015-04-22 19:00:53 UTC
Permalink
oVirt Jenkins CI Server has posted comments on this change.

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


Patch Set 1:

Build Started (2/2) -> http://jenkins.ovirt.org/job/ovirt-engine_master_find-bugs_gerrit/32516/
--
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: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
a***@ovirt.org
2015-04-22 19:18:02 UTC
Permalink
***@ovirt.org has posted comments on this change.

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


Patch Set 2:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-engine-3.5', 'ovirt-engine-3.4', 'ovirt-engine-3.3', 'ovirt-engine-3.5.2', 'ovirt-engine-3.4.0', 'ovirt-engine-3.3.4', 'ovirt-engine-3.3.3', 'ovirt-engine-3.3.2', 'ovirt-engine-3.3.1'])
--
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: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
oVirt Jenkins CI Server
2015-04-22 19:18:08 UTC
Permalink
oVirt Jenkins CI Server has posted comments on this change.

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


Patch Set 2:

Build Started (1/2) -> http://jenkins.ovirt.org/job/ovirt-engine_master_unit-tests_gerrit/33058/
--
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: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
oVirt Jenkins CI Server
2015-04-22 19:18:09 UTC
Permalink
oVirt Jenkins CI Server has posted comments on this change.

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


Patch Set 2:

Build Started (2/2) -> http://jenkins.ovirt.org/job/ovirt-engine_master_find-bugs_gerrit/32517/
--
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: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
oVirt Jenkins CI Server
2015-04-22 19:33:12 UTC
Permalink
oVirt Jenkins CI Server has posted comments on this change.

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


Patch Set 1: Code-Review-1 Verified-1

Build Unstable

http://jenkins.ovirt.org/job/ovirt-engine_master_unit-tests_gerrit/33056/ : SUCCESS

http://jenkins.ovirt.org/job/ovirt-engine_master_find-bugs_gerrit/32516/ : The patch does not pass the findbugs
--
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: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
oVirt Jenkins CI Server
2015-04-22 19:42:08 UTC
Permalink
oVirt Jenkins CI Server has posted comments on this change.

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


Patch Set 2:

Build Successful

http://jenkins.ovirt.org/job/ovirt-engine_master_unit-tests_gerrit/33058/ : SUCCESS

http://jenkins.ovirt.org/job/ovirt-engine_master_find-bugs_gerrit/32517/ : SUCCESS
--
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: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
Alon Bar-Lev
2015-04-22 20:35:37 UTC
Permalink
Alon Bar-Lev has posted comments on this change.

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


Patch Set 2:

(4 comments)

https://gerrit.ovirt.org/#/c/40157/2/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 65: public Map<String, Object> parse(List<String> args) {
Line 66: List<Object> others = new ArrayList<>();
Line 67: List<Throwable> errors = new ArrayList<>();
Line 68: Map<String, Object> argMap = new HashMap<>();
Line 69: args.addAll(0, defaults);
this is not good, as it does not go vie the convertor....
Line 70:
Line 71: while(!args.isEmpty()) {
Line 72: String arg = args.get(0);
Line 73: if(!arg.startsWith(LONG_PREFIX)) {


Line 109: new IllegalArgumentException(
Line 110: String.format("Value is required, but missing for argument '%1$s'", key)
Line 111: )
Line 112: );
Line 113: }
.

if (value == null) {
value = parserArgument.getValue();
}
Line 114: Object convertedValue = null;
Line 115: if (value != null) {
Line 116: Matcher m = parserArgument.getMatcher().matcher(value);
Line 117: if (!m.find()) {


https://gerrit.ovirt.org/#/c/40157/2/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 1: arg.mandatory = false
add comments... so we remember what each is... :)
Line 2: arg.help = no help for this argument
Line 3: arg.type = no_argument
Line 4: arg.convert = java.lang.String
Line 5: arg.matcher = .*


Line 3: arg.type = no_argument
Line 4: arg.convert = java.lang.String
Line 5: arg.matcher = .*
Line 6: arg.metavar = STRING
Line 7: arg.multivalue = false
.

# if argument exist but has no value, set this value
# arg.value =
# set this value as default
# arg.default =
Line 8: help.usage =
Line 9: help.header =
--
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: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
Alon Bar-Lev
2015-04-22 20:47:05 UTC
Permalink
Alon Bar-Lev has posted comments on this change.

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


Patch Set 2:

(2 comments)

https://gerrit.ovirt.org/#/c/40157/2/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 65: public Map<String, Object> parse(List<String> args) {
Line 66: List<Object> others = new ArrayList<>();
Line 67: List<Throwable> errors = new ArrayList<>();
Line 68: Map<String, Object> argMap = new HashMap<>();
Line 69: args.addAll(0, defaults);
Not sure what you mean. It goes via convertor in parseProperties.
can you please refer me where the default is actually converted?
Line 70:
Line 71: while(!args.isEmpty()) {
Line 72: String arg = args.get(0);
Line 73: if(!arg.startsWith(LONG_PREFIX)) {


Line 196: try {
Line 197: argument.setConvert(
Line 198: // default value for no_argument is boolean, for every other it's String
Line 199: argument.getType() != ArgumentType.no_argument ?
Line 200: Boolean.class
this can be set by user
Line 201: :
Line 202: Class.forName(
Line 203: getArgAttrValue(arg, "convert"),
Line 204: true,
--
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: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
Alon Bar-Lev
2015-04-22 21:01:23 UTC
Permalink
Alon Bar-Lev has posted comments on this change.

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


Patch Set 2:

(1 comment)

https://gerrit.ovirt.org/#/c/40157/2/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 65: public Map<String, Object> parse(List<String> args) {
Line 66: List<Object> others = new ArrayList<>();
Line 67: List<Throwable> errors = new ArrayList<>();
Line 68: Map<String, Object> argMap = new HashMap<>();
Line 69: args.addAll(0, defaults);
I can explain how it works, maybe I don't exactly understand your comment.
oh! now I understand the:

defaults.add(String.format("%1$s%2$s=%3$s", LONG_PREFIX, argument.getName(), argumen
t.getDefaultValue()));


although this is an interesting idea!

for multi value it will add the default as first entry... which is not correct...
Line 70:
Line 71: while(!args.isEmpty()) {
Line 72: String arg = args.get(0);
Line 73: if(!arg.startsWith(LONG_PREFIX)) {
--
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: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
Alon Bar-Lev
2015-04-22 21:18:33 UTC
Permalink
Alon Bar-Lev has posted comments on this change.

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


Patch Set 2:

(1 comment)

https://gerrit.ovirt.org/#/c/40157/2/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 65: public Map<String, Object> parse(List<String> args) {
Line 66: List<Object> others = new ArrayList<>();
Line 67: List<Throwable> errors = new ArrayList<>();
Line 68: Map<String, Object> argMap = new HashMap<>();
Line 69: args.addAll(0, defaults);
better to parse and then complete argMap with the defaults.
Line 70:
Line 71: while(!args.isEmpty()) {
Line 72: String arg = args.get(0);
Line 73: if(!arg.startsWith(LONG_PREFIX)) {
--
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: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
a***@ovirt.org
2015-04-24 11:54:15 UTC
Permalink
***@ovirt.org has posted comments on this change.

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


Patch Set 3:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-engine-3.5', 'ovirt-engine-3.4', 'ovirt-engine-3.3', 'ovirt-engine-3.5.2', 'ovirt-engine-3.4.0', 'ovirt-engine-3.3.4', 'ovirt-engine-3.3.3', 'ovirt-engine-3.3.2', 'ovirt-engine-3.3.1'])
--
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: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
oVirt Jenkins CI Server
2015-04-24 12:12:56 UTC
Permalink
oVirt Jenkins CI Server has posted comments on this change.

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


Patch Set 3:

Build Started (1/2) -> http://jenkins.ovirt.org/job/ovirt-engine_master_unit-tests_gerrit/33159/
--
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: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
oVirt Jenkins CI Server
2015-04-24 12:47:30 UTC
Permalink
oVirt Jenkins CI Server has posted comments on this change.

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


Patch Set 3:

Build Started (2/2) -> http://jenkins.ovirt.org/job/ovirt-engine_master_find-bugs_gerrit/32619/
--
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: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
oVirt Jenkins CI Server
2015-04-24 13:22:10 UTC
Permalink
oVirt Jenkins CI Server has posted comments on this change.

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


Patch Set 3:

Build Successful

http://jenkins.ovirt.org/job/ovirt-engine_master_unit-tests_gerrit/33159/ : SUCCESS

http://jenkins.ovirt.org/job/ovirt-engine_master_find-bugs_gerrit/32619/ : SUCCESS
--
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: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
Alon Bar-Lev
2015-04-26 12:18:01 UTC
Permalink
Alon Bar-Lev has posted comments on this change.

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


Patch Set 3:

(3 comments)

https://gerrit.ovirt.org/#/c/40157/3/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 107: new IllegalArgumentException(
Line 108: String.format("Value is required, but missing for argument '%1$s'", key)
Line 109: )
Line 110: );
Line 111: }
here
Line 112: Object convertedValue = null;
Line 113: if (value != null) {
Line 114: Matcher m = parserArgument.getMatcher().matcher(value);
Line 115: if (!m.find()) {


Line 123: }
Line 124: convertedValue = Util.getObjectValueByString(parserArgument.getConvert(), value);
Line 125: } else if (parserArgument.getType() == ArgumentType.no_argument) {
Line 126: convertedValue = Boolean.TRUE;
Line 127: }
why not just put the value as raw and have developer put the Boolean converter?

this will enable us to do this much generic.

no_argument converter is set to Boolean and value set to True.

this means that we have no value so we assign "True" and then process the converter as usual and get True.

this will enable us to also have optional_argument with default value, as --debug will for example put 5 as default while --debug=8 will leave 8.

so before this matcher conditional there should be:

if (value == null && parserArgument.getValue() != null) {
value = parserArgument.getValue();
}

and regular flow should follow.
Line 128:
Line 129: if (!parserArgument.isMultivalue()) {
Line 130: argMap.put(key, convertedValue);
Line 131: } else {


Line 166: );
Line 167: }
Line 168: if (arg.getType() == ArgumentType.no_argument) {
Line 169: argMap.put(arg.getName(), arg.isValue());
Line 170: }
not sure why we need this conditional.

if I put no_argument as default False, value True and converter Boolean I can achieve the same in properties file without anything in special in implementation, no?
Line 171: }
Line 172: }
Line 173: }
Line 174:
--
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: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
Alon Bar-Lev
2015-04-26 12:23:44 UTC
Permalink
Alon Bar-Lev has posted comments on this change.

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


Patch Set 3:

(1 comment)

https://gerrit.ovirt.org/#/c/40157/3/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 is of type no_argument set it value arg.value and convert to java.lang.Boolean if argument is not provided
Line 19: # if argument is provided set it's value to true
Line 20: arg.value = false
this should be commented so default will be null

not sure I understand this comment... this value should be set based on converter when value is not available, for example: no_argument or optional_arguments when value was not provided, so in summary when value is not provided this value should be set... as required_argument must have value anyway...
Line 21: # if argument is mandatory and not specified set it's value to arg.default, by default it's none,
Line 22: # so if argument is mandatory and has no default value parsing will fail on IllegalArgumentException
Line 23: #arg.default =
Line 24: # will be print as first, should contain usage of 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: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
a***@ovirt.org
2015-04-28 21:58:43 UTC
Permalink
***@ovirt.org has posted comments on this change.

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


Patch Set 4:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-engine-3.5', 'ovirt-engine-3.4', 'ovirt-engine-3.3', 'ovirt-engine-3.5.2', 'ovirt-engine-3.4.0', 'ovirt-engine-3.3.4', 'ovirt-engine-3.3.3', 'ovirt-engine-3.3.2', 'ovirt-engine-3.3.1'])
--
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: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
oVirt Jenkins CI Server
2015-04-28 21:58:52 UTC
Permalink
oVirt Jenkins CI Server has posted comments on this change.

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


Patch Set 4:

Build Started (1/2) -> http://jenkins.ovirt.org/job/ovirt-engine_master_unit-tests_gerrit/33742/
--
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: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
oVirt Jenkins CI Server
2015-04-28 21:58:57 UTC
Permalink
oVirt Jenkins CI Server has posted comments on this change.

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


Patch Set 4:

Build Started (2/2) -> http://jenkins.ovirt.org/job/ovirt-engine_master_find-bugs_gerrit/33204/
--
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: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
Alon Bar-Lev
2015-04-28 22:01:42 UTC
Permalink
Alon Bar-Lev has posted comments on this change.

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


Patch Set 4:

(1 comment)

last one! :)

https://gerrit.ovirt.org/#/c/40157/4/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 110: );
Line 111: }
Line 112: if (value == null && parserArgument.getValue() != null) {
Line 113: value = parserArgument.getValue();
Line 114: }
actually it can be reduced to:

if (value == null) {
value = parserArgument.getValue();
}

:)
Line 115: Object convertedValue = null;
Line 116: if (value != null) {
Line 117: Matcher m = parserArgument.getMatcher().matcher(value);
Line 118: if (!m.find()) {
--
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: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
m***@gmail.com
2015-04-28 22:04:48 UTC
Permalink
Ondřej Macháček has posted comments on this change.

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


Patch Set 4:

(1 comment)

https://gerrit.ovirt.org/#/c/40157/4/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 110: );
Line 111: }
Line 112: if (value == null && parserArgument.getValue() != null) {
Line 113: value = parserArgument.getValue();
Line 114: }
true:)
Line 115: Object convertedValue = null;
Line 116: if (value != null) {
Line 117: Matcher m = parserArgument.getMatcher().matcher(value);
Line 118: if (!m.find()) {
--
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: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
a***@ovirt.org
2015-04-28 22:06:38 UTC
Permalink
***@ovirt.org has posted comments on this change.

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


Patch Set 5:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-engine-3.5', 'ovirt-engine-3.4', 'ovirt-engine-3.3', 'ovirt-engine-3.5.2', 'ovirt-engine-3.4.0', 'ovirt-engine-3.3.4', 'ovirt-engine-3.3.3', 'ovirt-engine-3.3.2', 'ovirt-engine-3.3.1'])
--
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: 5
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
oVirt Jenkins CI Server
2015-04-28 22:06:41 UTC
Permalink
oVirt Jenkins CI Server has posted comments on this change.

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


Patch Set 5:

Build Started (1/2) -> http://jenkins.ovirt.org/job/ovirt-engine_master_unit-tests_gerrit/33743/
--
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: 5
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
oVirt Jenkins CI Server
2015-04-28 22:06:43 UTC
Permalink
oVirt Jenkins CI Server has posted comments on this change.

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


Patch Set 5:

Build Started (2/2) -> http://jenkins.ovirt.org/job/ovirt-engine_master_find-bugs_gerrit/33207/
--
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: 5
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
oVirt Jenkins CI Server
2015-04-28 22:29:37 UTC
Permalink
oVirt Jenkins CI Server has posted comments on this change.

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


Patch Set 4:

Build Successful

http://jenkins.ovirt.org/job/ovirt-engine_master_unit-tests_gerrit/33742/ : SUCCESS

http://jenkins.ovirt.org/job/ovirt-engine_master_find-bugs_gerrit/33204/ : SUCCESS
--
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: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
oVirt Jenkins CI Server
2015-04-28 22:30:35 UTC
Permalink
oVirt Jenkins CI Server has posted comments on this change.

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


Patch Set 5:

Build Successful

http://jenkins.ovirt.org/job/ovirt-engine_master_unit-tests_gerrit/33743/ : SUCCESS

http://jenkins.ovirt.org/job/ovirt-engine_master_find-bugs_gerrit/33207/ : SUCCESS
--
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: 5
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
Alon Bar-Lev
2015-04-28 22:34:55 UTC
Permalink
Alon Bar-Lev has posted comments on this change.

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


Patch Set 5: Code-Review+1
--
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: 5
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
Alon Bar-Lev
2015-04-28 23:15:16 UTC
Permalink
Alon Bar-Lev has posted comments on this change.

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


Patch Set 5: -Code-Review

(1 comment)

https://gerrit.ovirt.org/#/c/40157/5/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 163: if (arg.getDefaultValue() != null) {
Line 164: argMap.put(
Line 165: arg.getName(),
Line 166: Util.getObjectValueByString(arg.getConvert(), arg.getDefaultValue())
Line 167: );
sorry... forgot... if multi value we need to create a collection.
Line 168: }
Line 169: }
Line 170: }
Line 171: }
--
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: 5
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
a***@ovirt.org
2015-04-29 08:30:23 UTC
Permalink
***@ovirt.org has posted comments on this change.

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


Patch Set 6:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-engine-3.5', 'ovirt-engine-3.4', 'ovirt-engine-3.3', 'ovirt-engine-3.5.2', 'ovirt-engine-3.4.0', 'ovirt-engine-3.3.4', 'ovirt-engine-3.3.3', 'ovirt-engine-3.3.2', 'ovirt-engine-3.3.1'])
--
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: 6
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
oVirt Jenkins CI Server
2015-04-29 08:30:26 UTC
Permalink
oVirt Jenkins CI Server has posted comments on this change.

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


Patch Set 6:

Build Started (1/2) -> http://jenkins.ovirt.org/job/ovirt-engine_master_unit-tests_gerrit/33763/
--
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: 6
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
oVirt Jenkins CI Server
2015-04-29 08:31:05 UTC
Permalink
oVirt Jenkins CI Server has posted comments on this change.

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


Patch Set 6:

Build Started (2/2) -> http://jenkins.ovirt.org/job/ovirt-engine_master_find-bugs_gerrit/33227/
--
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: 6
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
oVirt Jenkins CI Server
2015-04-29 08:56:20 UTC
Permalink
oVirt Jenkins CI Server has posted comments on this change.

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


Patch Set 6:

Build Successful

http://jenkins.ovirt.org/job/ovirt-engine_master_unit-tests_gerrit/33763/ : SUCCESS

http://jenkins.ovirt.org/job/ovirt-engine_master_find-bugs_gerrit/33227/ : SUCCESS
--
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: 6
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
a***@ovirt.org
2015-04-29 21:10:19 UTC
Permalink
***@ovirt.org has posted comments on this change.

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


Patch Set 7:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-engine-3.5', 'ovirt-engine-3.4', 'ovirt-engine-3.3', 'ovirt-engine-3.5.2', 'ovirt-engine-3.4.0', 'ovirt-engine-3.3.4', 'ovirt-engine-3.3.3', 'ovirt-engine-3.3.2', 'ovirt-engine-3.3.1'])
--
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: 7
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
oVirt Jenkins CI Server
2015-04-29 21:10:27 UTC
Permalink
oVirt Jenkins CI Server has posted comments on this change.

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


Patch Set 7:

Build Started (1/2) -> http://jenkins.ovirt.org/job/ovirt-engine_master_unit-tests_gerrit/33839/
--
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: 7
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
oVirt Jenkins CI Server
2015-04-29 21:10:29 UTC
Permalink
oVirt Jenkins CI Server has posted comments on this change.

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


Patch Set 7:

Build Started (2/2) -> http://jenkins.ovirt.org/job/ovirt-engine_master_find-bugs_gerrit/33287/
--
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: 7
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
m***@gmail.com
2015-04-29 21:12:19 UTC
Permalink
Ondřej Macháček has posted comments on this change.

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


Patch Set 6:

Changed the format of print help usage. Help of option is on new line, it should be nicer for longer help messages.

$ bin/engine-manage-domains edit --help
usage: bin/engine-manage-domains edit [options]
Edit an existing domain

Options:
--add-permissions
Add engine superuser permissions to the user.

--change-password-msg
Reads interactively a URL or a message to be returned to the user in case the password has expired

--config-file=[STRING]
Use the given alternate configuration file

--domain=[STRING]
The domain you wish to perform the action on.

--help
Show help

--ldap-servers=[STRING]
A comma delimited list of LDAP servers to be set to the domain.

--password-file=[STRING]
A file containing the password (if it's not set, the password will be read interactively)

--provider=[STRING]
The LDAP provider type of server used for the domain, can be one of (case insensitive):
ad Microsoft Active Directory
ipa freeIPA
rhds Red Hat Directory Server
itds IBM Tivoli Directory Server
oldap OpenLDAP

--resolve-kdc
Resolve KDC servers using DNS (don't assume they are the same as LDAP servers)

--user=[STRING]
The domain user

Example:
bin/engine-manage-domains edit --domain=DOMAIN [--provider=PROVIDER] [--user=USER] [--add-permissions] [--config-file=CFG_FILE] [--ldap-servers=SERVERS] [--resolve-kdc] [--password-file=PASS_FILE] [--change-password-msg]
--
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: 6
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
oVirt Jenkins CI Server
2015-04-29 21:36:05 UTC
Permalink
oVirt Jenkins CI Server has posted comments on this change.

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


Patch Set 7:

Build Successful

http://jenkins.ovirt.org/job/ovirt-engine_master_unit-tests_gerrit/33839/ : SUCCESS

http://jenkins.ovirt.org/job/ovirt-engine_master_find-bugs_gerrit/33287/ : SUCCESS
--
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: 7
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
Alon Bar-Lev
2015-04-30 05:55:08 UTC
Permalink
Alon Bar-Lev has posted comments on this change.

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


Patch Set 7:

(1 comment)

https://gerrit.ovirt.org/#/c/40157/7/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 277: for(String arg : getUsageArguments()) {
Line 278: ParserArgument argument = this.arguments.get(arg);
Line 279: help.append(
Line 280: String.format(
Line 281: " --%s%n %s%n%n",
please show the new line within the text:

(
" --%s%n" +
" %s%n" +
"%n"
),
Line 282: arg + (argument.getType() != ArgumentType.no_argument ? "=[" + argument.getMetavar() + "]" : ""),
Line 283: argument.getHelp()
Line 284: )
Line 285: );
--
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: 7
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
a***@ovirt.org
2015-04-30 07:28:04 UTC
Permalink
***@ovirt.org has posted comments on this change.

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


Patch Set 8:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-engine-3.5', 'ovirt-engine-3.4', 'ovirt-engine-3.3', 'ovirt-engine-3.5.2', 'ovirt-engine-3.4.0', 'ovirt-engine-3.3.4', 'ovirt-engine-3.3.3', 'ovirt-engine-3.3.2', 'ovirt-engine-3.3.1'])
--
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 <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
oVirt Jenkins CI Server
2015-04-30 07:28:07 UTC
Permalink
oVirt Jenkins CI Server has posted comments on this change.

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


Patch Set 8:

Build Started (1/2) -> http://jenkins.ovirt.org/job/ovirt-engine_master_unit-tests_gerrit/33848/
--
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 <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
oVirt Jenkins CI Server
2015-04-30 07:28:09 UTC
Permalink
oVirt Jenkins CI Server has posted comments on this change.

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


Patch Set 8:

Build Started (2/2) -> http://jenkins.ovirt.org/job/ovirt-engine_master_find-bugs_gerrit/33298/
--
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 <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
m***@gmail.com
2015-04-30 07:28:48 UTC
Permalink
Ondřej Macháček has posted comments on this change.

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


Patch Set 8: Verified+1
--
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 <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
m***@gmail.com
2015-04-30 07:31:47 UTC
Permalink
Ondřej Macháček has posted comments on this change.

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


Patch Set 8: -Verified
--
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 <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
m***@redhat.com
2015-04-30 08:01:15 UTC
Permalink
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 <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
oVirt Jenkins CI Server
2015-04-30 08:03:59 UTC
Permalink
oVirt Jenkins CI Server has posted comments on this change.

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


Patch Set 8:

Build Successful

http://jenkins.ovirt.org/job/ovirt-engine_master_unit-tests_gerrit/33848/ : SUCCESS

http://jenkins.ovirt.org/job/ovirt-engine_master_find-bugs_gerrit/33298/ : SUCCESS
--
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 <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
Alon Bar-Lev
2015-04-30 08:46:51 UTC
Permalink
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__";
Post by m***@redhat.com
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)
Post by m***@redhat.com
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);
Post by m***@redhat.com
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 <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
m***@redhat.com
2015-04-30 10:01:37 UTC
Permalink
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__";
Post by Alon Bar-Lev
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)
Post by Alon Bar-Lev
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);
Post by Alon Bar-Lev
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,
Post by Alon Bar-Lev
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 <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
Alon Bar-Lev
2015-04-30 10:05:27 UTC
Permalink
Alon Bar-Lev has posted comments on this change.

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


Patch Set 8:

(1 comment)

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__";
Post by m***@redhat.com
I think tail (or positional) is a better name for a new user of this class
positional is good for me, just like any... ondra?
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: */
--
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 <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
Alon Bar-Lev
2015-04-30 10:15:00 UTC
Permalink
Alon Bar-Lev has posted comments on this change.

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


Patch Set 8:

(1 comment)

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,
Post by m***@redhat.com
why not always take default?
difference between default and value:

default ddd
value vvv

for argument in all command line arguments:
if argument is required_argument and no value - error
if no value set value to vvv
for all parameters:
set ddd as value
if parameter is mandatory and no value - error

ondra, please confirm that mandatory behaves this way.
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 <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
m***@redhat.com
2015-04-30 10:39:40 UTC
Permalink
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 <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
m***@redhat.com
2015-04-30 12:50:57 UTC
Permalink
Martin Peřina 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 43:
Line 44: static enum ArgumentType {
Line 45: has_argument,
Line 46: optional_argument,
Line 47: no_argument,
OK, but I can't use MANDATORY, as such name is already used for different p
No problem
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) {
It was removed earlier, but IIRC Mooli wanted that.
I will talk to him about it, please remove
Line 57: this(loadProperties(resource), prefix);
Line 58: }
Line 59:
Line 60: public Map<String, Object> parse(String... args) {


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 {
Hmm, isn't this one of very common name ? :)
Yes, but you are in a parser subpackage, so it should be clear what is Argument about.
Line 6:
Line 7: private String name;
Line 8: private String help;
Line 9: private String defaultValue;


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 {
Well, this Util implements only one method which is not specific for Argume
I can't see its usage outside parser.

If this usage is planned, then please move this class outside parser package. As I don't see usage in other parts, if it seems ok for me to move it into uutils package and name it for example StringValueConverter
Line 15:
Line 16: private static final Map<Class<?>, Class<?>> typeBox = new HashMap<>();
Line 17: static {
Line 18: typeBox.put(boolean.class, Boolean.class);
--
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 <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
Alon Bar-Lev
2015-04-30 12:51:44 UTC
Permalink
Alon Bar-Lev has posted comments on this change.

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


Patch Set 8:

(1 comment)

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 89: if (
Line 90: value == null &&
Line 91: (
Line 92: parserArgument.getType() == ArgumentType.optional_argument ||
Line 93: parserArgument.getType() == ArgumentType.has_argument
I guess so
it is less clear what condition is all about.
Line 94: )
Line 95: ) {
Line 96: if(args.size() > 0) {
Line 97: value = args.get(0);
--
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 <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
Alon Bar-Lev
2015-04-30 12:53:52 UTC
Permalink
Alon Bar-Lev has posted comments on this change.

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


Patch Set 8:

(2 comments)

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 {
Post by m***@redhat.com
Yes, but you are in a parser subpackage, so it should be clear what is Argu
this class can be private anyway... it is not exposed so its name is not that important at this point.
Line 6:
Line 7: private String name;
Line 8: private String help;
Line 9: private String defaultValue;


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 {
Post by m***@redhat.com
I can't see its usage outside parser.
this can be package private.
Line 15:
Line 16: private static final Map<Class<?>, Class<?>> typeBox = new HashMap<>();
Line 17: static {
Line 18: typeBox.put(boolean.class, Boolean.class);
--
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 <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
Alon Bar-Lev
2015-04-30 13:50:12 UTC
Permalink
Alon Bar-Lev has posted comments on this change.

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


Patch Set 8:

(6 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 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 {
Post by m***@redhat.com
I would prefer ArgumentParser as it describes better that is parses command
Command line arguments or command line parameters or command line options...

for example:

getopt, getopt_long, getopt_long_only, optarg, optind, opterr, optopt - Parse command-line options

but then:

The getopt() function parses the command-line arguments.

not very important though...
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 43:
Line 44: static enum ArgumentType {
Line 45: has_argument,
Line 46: optional_argument,
Line 47: no_argument,
Post by m***@redhat.com
No problem
these are the terms getopt_long is using, I prefer keep it this way for us primitive people that needs references. it is internal to this class and not part of the api, so there is no discussion of java public type api.
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) {
Post by m***@redhat.com
I will talk to him about it, please remove
caller should be able to provide input stream as well to ease use with resources. caller can use the loadProperties directly so we set it as public or leave this as-is.
Line 57: this(loadProperties(resource), prefix);
Line 58: }
Line 59:
Line 60: public Map<String, Object> parse(String... args) {


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);
there is no state of this class, everything should get out of argMap.

if we want to have state, we can put the argMap, others and errors as members, but then it will be more difficult of reuse the instance.

I think current implementation is very simple.
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());
Post by m***@redhat.com
Please use List<?> instead of Collection
Collection<?> is better, as there is no reason to assume List.
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 105: if(obj == this) {
Line 106: return true;
Line 107: }
Line 108:
Line 109: return this.name.equals(((ParserArgument) obj).getName());
Post by m***@redhat.com
I was not aware of such util, thanks for info.
I am not sure what the difference is... martin, can you please explain?

return name.equals(other.name);
return Objects.equals(name, other.name);

For some reason, I find the 1st nicer.
Line 110: }
Line 111:
Line 112: @Override
Line 113: public int hashCode() {
--
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 <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
a***@ovirt.org
2015-04-30 15:55:43 UTC
Permalink
***@ovirt.org has posted comments on this change.

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


Patch Set 9:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-engine-3.5', 'ovirt-engine-3.4', 'ovirt-engine-3.3', 'ovirt-engine-3.5.2', 'ovirt-engine-3.4.0', 'ovirt-engine-3.3.4', 'ovirt-engine-3.3.3', 'ovirt-engine-3.3.2', 'ovirt-engine-3.3.1'])
--
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: 9
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
oVirt Jenkins CI Server
2015-04-30 15:59:43 UTC
Permalink
oVirt Jenkins CI Server has posted comments on this change.

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


Patch Set 9:

Build Started (1/2) -> http://jenkins.ovirt.org/job/ovirt-engine_master_unit-tests_gerrit/33943/
--
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: 9
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
Alon Bar-Lev
2015-04-30 16:14:56 UTC
Permalink
Alon Bar-Lev has posted comments on this change.

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


Patch Set 9:

(3 comments)

https://gerrit.ovirt.org/#/c/40157/9/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 8: * This is mapping class from argument defined in properties file to java class.
Line 9: * Attributes of this class is 1:1 mapping described here {@link org.ovirt.engine.core.uutils.cli.parser.ArgumentsParser}
Line 10: *
Line 11: */
Line 12: public class Argument {
public can be dropped.
Line 13:
Line 14: private String name;
Line 15: private String help;
Line 16: private String defaultValue;


Line 116: if (!(obj instanceof Argument)) {
Line 117: return false;
Line 118: }
Line 119:
Line 120: return Objects.equals(name, ((Argument) obj).getName());
while you are at this, you do not need getName() as you can access the members of other directly.

and it can be:

return obj instanceof Argument && Objects.equals(name, ((Argument) obj).name);

but I still do not understand what's the difference of:

return obj instanceof Argument && name.equals(((Argument) obj).name);

maybe if name is null(?!)
Line 121: }
Line 122:
Line 123: @Override
Line 124: public int hashCode() {


https://gerrit.ovirt.org/#/c/40157/9/backend/manager/modules/uutils/src/main/java/org/ovirt/engine/core/uutils/cli/parser/StringValueConverter.java
File backend/manager/modules/uutils/src/main/java/org/ovirt/engine/core/uutils/cli/parser/StringValueConverter.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 StringValueConverter {
public can be dropped
Line 15:
Line 16: private static final Map<Class<?>, Class<?>> typeBox = new HashMap<>();
Line 17: static {
Line 18: typeBox.put(boolean.class, Boolean.class);
--
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: 9
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
Alon Bar-Lev
2015-04-30 16:21:58 UTC
Permalink
Alon Bar-Lev has posted comments on this change.

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


Patch Set 9:

(2 comments)

https://gerrit.ovirt.org/#/c/40157/9/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 202: String arg = args.get(argumentIndex);
Line 203: if(!arg.startsWith(LONG_PREFIX)) {
Line 204: break;
Line 205: }
Line 206: argumentIndex++;
just create a copy of args, and revert the use of index, very difficult now to review diff anyway.

what is the advantage of using the index?
Line 207: if(arg.equals(LONG_PREFIX)) {
Line 208: break;
Line 209: }
Line 210:


Line 300: private void putValue(Map<String, Object> argMap, Argument arg, Object value) {
Line 301: if (!arg.isMultivalue()) {
Line 302: argMap.put(arg.getName(), value);
Line 303: } else {
Line 304: List<? super Object> c = (List)argMap.get(arg.getName());
this should be a collection unless a good reason.
Line 305: if (c == null) {
Line 306: c = new ArrayList<>();
Line 307: argMap.put(arg.getName(), c);
Line 308: }
--
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: 9
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
Alon Bar-Lev
2015-04-30 16:22:44 UTC
Permalink
Alon Bar-Lev has posted comments on this change.

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


Patch Set 9:

(1 comment)

https://gerrit.ovirt.org/#/c/40157/9/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 7: *
Line 8: * This is mapping class from argument defined in properties file to java class.
Line 9: * Attributes of this class is 1:1 mapping described here {@link org.ovirt.engine.core.uutils.cli.parser.ArgumentsParser}
Line 10: *
Line 11: */
should not be documented, has no value as it is private
Line 12: public class Argument {
Line 13:
Line 14: private String name;
Line 15: private String help;
--
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: 9
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
Alon Bar-Lev
2015-04-30 16:27:13 UTC
Permalink
Alon Bar-Lev has posted comments on this change.

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


Patch Set 9:

I just thought...

we can revert the index and redo the collection modification.

then remove the positional from the arguments and just state that the function 'eats' the parameters it parses, and leave the args with remaining.

this should be consistence with the getopt as well, and I think will be nicer.
--
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: 9
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
oVirt Jenkins CI Server
2015-04-30 16:41:42 UTC
Permalink
oVirt Jenkins CI Server has posted comments on this change.

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


Patch Set 9:

Build Started (2/2) -> http://jenkins.ovirt.org/job/ovirt-engine_master_find-bugs_gerrit/33395/
--
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: 9
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
oVirt Jenkins CI Server
2015-04-30 17:07:52 UTC
Permalink
oVirt Jenkins CI Server has posted comments on this change.

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


Patch Set 9:

Build Successful

http://jenkins.ovirt.org/job/ovirt-engine_master_unit-tests_gerrit/33943/ : SUCCESS

http://jenkins.ovirt.org/job/ovirt-engine_master_find-bugs_gerrit/33395/ : SUCCESS
--
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: 9
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
a***@ovirt.org
2015-04-30 17:59:42 UTC
Permalink
***@ovirt.org has posted comments on this change.

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


Patch Set 10:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-engine-3.5', 'ovirt-engine-3.4', 'ovirt-engine-3.3', 'ovirt-engine-3.5.2', 'ovirt-engine-3.4.0', 'ovirt-engine-3.3.4', 'ovirt-engine-3.3.3', 'ovirt-engine-3.3.2', 'ovirt-engine-3.3.1'])
--
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: 10
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
oVirt Jenkins CI Server
2015-04-30 17:59:45 UTC
Permalink
oVirt Jenkins CI Server has posted comments on this change.

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


Patch Set 10:

Build Started (1/2) -> http://jenkins.ovirt.org/job/ovirt-engine_master_unit-tests_gerrit/33962/
--
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: 10
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
oVirt Jenkins CI Server
2015-04-30 17:59:46 UTC
Permalink
oVirt Jenkins CI Server has posted comments on this change.

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


Patch Set 10:

Build Started (2/2) -> http://jenkins.ovirt.org/job/ovirt-engine_master_find-bugs_gerrit/33414/
--
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: 10
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
oVirt Jenkins CI Server
2015-04-30 18:07:24 UTC
Permalink
oVirt Jenkins CI Server has posted comments on this change.

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


Patch Set 10: Code-Review-1 Verified-1

Build Failed

http://jenkins.ovirt.org/job/ovirt-engine_master_unit-tests_gerrit/33962/ : FAILURE

http://jenkins.ovirt.org/job/ovirt-engine_master_find-bugs_gerrit/33414/ : The patch does not pass the findbugs
--
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: 10
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
Alon Bar-Lev
2015-04-30 18:08:16 UTC
Permalink
Alon Bar-Lev has posted comments on this change.

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


Patch Set 10:

(1 comment)

https://gerrit.ovirt.org/#/c/40157/10/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 183: * @param cmdArgs array of command line arguments
Line 184: * @return Map of correctly parsed and converted arguments
Line 185: */
Line 186: public Map<String, Object> parse(String... cmdArgs) {
Line 187: return parse(Arrays.asList(cmdArgs));
what about my idea of eating the arguments we parse and remove the __positional__ from the argument map?
Line 188: }
Line 189:
Line 190: /**
Line 191: * Parse list of arguments based on definition declared in {@link #properties} file with {@link #prefix}.
--
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: 10
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
Alon Bar-Lev
2015-04-30 18:08:26 UTC
Permalink
Alon Bar-Lev has posted comments on this change.

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


Patch Set 9:

(1 comment)

https://gerrit.ovirt.org/#/c/40157/9/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 300: private void putValue(Map<String, Object> argMap, Argument arg, Object value) {
Line 301: if (!arg.isMultivalue()) {
Line 302: argMap.put(arg.getName(), value);
Line 303: } else {
Line 304: List<? super Object> c = (List)argMap.get(arg.getName());
Hmm, Martin suggested List<?>. I don't know what's better, but I think Coll
there is no reason for using specific class if super class is appropriate.
Line 305: if (c == null) {
Line 306: c = new ArrayList<>();
Line 307: argMap.put(arg.getName(), c);
Line 308: }
--
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: 9
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
Alon Bar-Lev
2015-04-30 19:00:39 UTC
Permalink
Alon Bar-Lev has posted comments on this change.

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


Patch Set 10:

(3 comments)

just to revisit the idea of args that can be modified...

it simplifies the processing of actions and such, no need to get the others...

parse(args);
action = args.remove(0);
parse(args);

https://gerrit.ovirt.org/#/c/40157/10/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 27: * <p>
Line 28: * name - Name of the argument, <b>Must be declared by developer</b>
Line 29: * help - Help to be printed to argument, when user request help to be printed (default: empty string)
Line 30: * mandatory - true/false declares if argument have to be specified or not (default: false)
Line 31: * type - one of:
if you do javadoc these should be probably:

<ul>
<li>xxx</li>
<li>xxx</li>
</ul>
Line 32: * has_argument - argument requires value
Line 33: * optional_argument - argument could have value
Line 34: * no_argument - argument doesn't have value (default)
Line 35: * convert - Name of the class that arguments value should be converted to (default: java.lang.String)


Line 70: * --<prefix>arg.argument2.name
Line 71: * <prefix>arg.argument2.help
Line 72: *
Line 73: * $help.footer
Line 74: * """
I think this should be in <pre>

<pre>{@code
bla bla bla
}</pre>
Line 75: * </p>
Line 76: *
Line 77: * Example:
Line 78: * We have program which support two actions - [add, remove]. Both actions accepts different arguments and program too.


Line 127: * Prefix which every argument should use to be considered argument.
Line 128: */
Line 129: private static final String LONG_PREFIX = "--";
Line 130:
Line 131: /**
no need to javadoc private unless you just want to work hard :)
Line 132: * Stores default values of every argument, if user don't override it default from this file will be used.
Line 133: */
Line 134: private static final Properties defaultProperties = loadProperties(
Line 135: ArgumentsParser.class.getResourceAsStream("defaults.properties")
--
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: 10
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
Alon Bar-Lev
2015-04-30 19:11:57 UTC
Permalink
Alon Bar-Lev has posted comments on this change.

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


Patch Set 10:

instead of copying the args, we can just remove these args that we process, leaving the args with only the leftovers.

this will enable us to remove what was once __other__ from the return value of parser.
--
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: 10
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
Alon Bar-Lev
2015-04-30 19:59:14 UTC
Permalink
Alon Bar-Lev has posted comments on this change.

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


Patch Set 10:

if you want to remove errors from the args, I wounder if not better to add parameter (collection) for errors, if not provided or null we throw exception of first error.

errors = new ArrayList<Throwable>();
parse(args, errors);

or:

parse(args);

but if you do hold the errors as member, then there is no reason to return anything from parse, so that args are also a member, this will be consistent. parse can return boolean to mark existence of errors.
--
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: 10
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
m***@redhat.com
2015-04-30 21:58:54 UTC
Permalink
Martin Peřina has posted comments on this change.

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


Patch Set 8:

(2 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 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());
Post by Alon Bar-Lev
Done
There's no sense to use Collection when you insert ArrayList instance as a value
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 105: if(obj == this) {
Line 106: return true;
Line 107: }
Line 108:
Line 109: return this.name.equals(((ParserArgument) obj).getName());
Post by Alon Bar-Lev
I am not sure what the difference is... martin, can you please explain?
You should check for nullability to prevent NPE if you use

name.equals(other.name);

Objects.equals already contains this test, so it can be used in all cases.
Line 110: }
Line 111:
Line 112: @Override
Line 113: public int hashCode() {
--
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 <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-HasComments: Yes
Alon Bar-Lev
2015-04-30 22:03:33 UTC
Permalink
Alon Bar-Lev has posted comments on this change.

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


Patch Set 8:

(1 comment)

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 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());
Post by m***@redhat.com
There's no sense to use Collection when you insert ArrayList instance as a
there is no sense of creating dependencies within two unrelated code branches. the ArrayList is a collection implementation, which can be modified at any time. the interface is a Collection to add new elements.
Line 167: if (c == null) {
Line 168: c = new ArrayList<>();
Line 169: argMap.put(arg.getName(), c);
Line 170: }
--
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 <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-HasComments: Yes
m***@redhat.com
2015-05-05 08:20:58 UTC
Permalink
mooli tayer has posted comments on this change.

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


Patch Set 10:

(3 comments)

https://gerrit.ovirt.org/#/c/40157/10/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 74: * """
Line 75: * </p>
Line 76: *
Line 77: * Example:
Line 78: * We have program which support two actions - [add, remove]. Both actions accepts different arguments and program too.
s/program/a program/
s/Both actions accepts/Both actions accept

also I don't understand the "and program too" part.
Line 79: * ./out [--file] add --message=X [--index]
Line 80: * ./out [--file] remove [--index]
Line 81: *
Line 82: * The properties file of this program can look like:


Line 80: * ./out [--file] remove [--index]
Line 81: *
Line 82: * The properties file of this program can look like:
Line 83: * module.arg.file.name = file
Line 84: * module.arg.file.help = File where store message
s/File where store message/file where messages are stored/
Line 85: * module.arg.file.default = /tmp/X
Line 86: * add.arg.message.name = message
Line 87: * add.arg.message.help = Message to be stored
Line 88: * add.arg.message.mandatory = true


Line 90: * add.arg.index.name = index
Line 91: * add.arg.index.help = Index where message should be inserted
Line 92: * add.arg.index.value = 0
Line 93: * add.arg.index.default = 0
Line 94: * add.arg.index.type = OPTIONAL
sorry I still don't get it...

"if argument is mandatory and if argument is not specified on command line then set value which is stored in arg.default"

this means afaiu that 'default' has no meaning if arg is not mandatory. (which I also don't understand why)
Line 95: * remove.arg.index.name = index
Line 96: * remove.arg.index.help = Index where message should be inserted
Line 97: * remove.arg.index.value = 0
Line 98: * remove.arg.index.default = 0
--
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: 10
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-HasComments: Yes
m***@redhat.com
2015-05-05 09:36:37 UTC
Permalink
mooli tayer has posted comments on this change.

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


Patch Set 10:

(1 comment)

https://gerrit.ovirt.org/#/c/40157/10/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 273: )
Line 274: );
Line 275: }
Line 276: others.addAll(args);
Line 277: argMap.put(PARAMETER_KEY_POSITIONAL, others);
you can remove the two lines above as well as
List<Object> others = new ArrayList<>();
and instead do:
argMap.put(PARAMETER_KEY_POSITIONAL, args);

AFAIK this is what Alon's comment on line 187 means
Line 278: argMap.put(PARAMETER_KEY_ERRORS, errors);
Line 279:
Line 280: return argMap;
Line 281: }
--
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: 10
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-HasComments: Yes
m***@redhat.com
2015-05-05 19:11:02 UTC
Permalink
mooli tayer has posted comments on this change.

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


Patch Set 10:

(1 comment)

https://gerrit.ovirt.org/#/c/40157/10/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 221: } else {
Line 222: if (
Line 223: value == null &&
Line 224: (
Line 225: argument.getType() != Argument.Type.OPTIONAL_ARGUMENT ||
should be ==?
Line 226: argument.getType() == Argument.Type.HAS_ARGUMENT
Line 227: )
Line 228: ) {
Line 229: if(args.size() > 0) {
--
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: 10
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-HasComments: Yes
Alon Bar-Lev
2015-05-05 19:14:15 UTC
Permalink
Alon Bar-Lev has posted comments on this change.

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


Patch Set 10:

(1 comment)

https://gerrit.ovirt.org/#/c/40157/10/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 221: } else {
Line 222: if (
Line 223: value == null &&
Line 224: (
Line 225: argument.getType() != Argument.Type.OPTIONAL_ARGUMENT ||
Post by m***@redhat.com
should be ==?
yes, correct.
Line 226: argument.getType() == Argument.Type.HAS_ARGUMENT
Line 227: )
Line 228: ) {
Line 229: if(args.size() > 0) {
--
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: 10
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-HasComments: Yes
a***@ovirt.org
2015-05-08 07:34:43 UTC
Permalink
***@ovirt.org has posted comments on this change.

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


Patch Set 11:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-engine-3.5', 'ovirt-engine-3.4', 'ovirt-engine-3.3', 'ovirt-engine-3.5.2', 'ovirt-engine-3.4.0', 'ovirt-engine-3.3.4', 'ovirt-engine-3.3.3', 'ovirt-engine-3.3.2', 'ovirt-engine-3.3.1'])
--
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: 11
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-HasComments: No
Alon Bar-Lev
2015-05-08 10:16:07 UTC
Permalink
Alon Bar-Lev has posted comments on this change.

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


Patch Set 11:

(2 comments)

https://gerrit.ovirt.org/#/c/40157/11/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 156:
Line 157: /**
Line 158: * Map of correctly parsed and converted arguments
Line 159: */
Line 160: private Map<String, Object> argMap = new HashMap<>();
parsedArgs?
Line 161:
Line 162: /**
Line 163: * List of errors which was found during pasring
Line 164: */


Line 193: *
Line 194: * @param args list of command line arguments
Line 195: * @return true if parsing has no errors, false otherwise
Line 196: */
Line 197: public boolean parse(List<String> args) {
must clear errors and argMap to start a new clean sequence.
Line 198: while(!args.isEmpty()) {
Line 199: String arg = args.get(0);
Line 200: if(!arg.startsWith(LONG_PREFIX)) {
Line 201: break;
--
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: 11
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-HasComments: Yes
Alon Bar-Lev
2015-05-08 10:31:10 UTC
Permalink
Alon Bar-Lev has posted comments on this change.

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


Patch Set 11:

(1 comment)

https://gerrit.ovirt.org/#/c/40157/11/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 193: *
Line 194: * @param args list of command line arguments
Line 195: * @return true if parsing has no errors, false otherwise
Line 196: */
Line 197: public boolean parse(List<String> args) {
Post by Alon Bar-Lev
must clear errors and argMap to start a new clean sequence.
set these to null as members, and assign new collections here.
Line 198: while(!args.isEmpty()) {
Line 199: String arg = args.get(0);
Line 200: if(!arg.startsWith(LONG_PREFIX)) {
Line 201: break;
--
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: 11
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-HasComments: Yes
Alon Bar-Lev
2015-05-08 11:04:49 UTC
Permalink
Alon Bar-Lev has posted comments on this change.

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


Patch Set 11:

(4 comments)

https://gerrit.ovirt.org/#/c/40157/11/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 130: /**
Line 131: * Stores default values of every argument, if user don't override it default from this file will be used.
Line 132: */
Line 133: private static final Properties defaultProperties = loadProperties(
Line 134: ArgumentsParser.class.getResourceAsStream("defaults.properties")
question: shouldn't we close this stream?

private static final Properties defaultProperties;
static {
try(InputStream is = ArgumentsParser.class.getResourceAsStream("defaults.properties")) {
defaultProperties = loadProperties(is);
}
}
Line 135: );
Line 136:
Line 137: /**
Line 138: * Stores user defined arguments in properties file.


Line 278: *
Line 279: * @return list of erros
Line 280: */
Line 281: public List<Throwable> getErrors() {
Line 282: return errors;
I hope you do not kill me, but as this is infra component it is good to know the practices...

you should not allow caller to modify collection.

return Collections.unmodifiableList(errors);
Line 283: }
Line 284:
Line 285: /**
Line 286: * Return map of validated and parsed arguments


Line 287: *
Line 288: * @return Map of validated and parsed arguments
Line 289: */
Line 290: public Map<String, Object> getArgMap() {
Line 291: return argMap;
same...

return Collections.unmodifiableMap(argMap);
Line 292: }
Line 293:
Line 294: /**
Line 295: * Go through all arguments which has default value. If such argument is not in {@code argMap} put it there.


Line 454: * $prefix.help.footer
Line 455: *
Line 456: * @return formatted string with usage
Line 457: */
Line 458: public String getUsage() {
usually it is best to have all public together at bottom or top (depending on taste).
Line 459: StringBuilder help = new StringBuilder(String.format("Options:%n"));
Line 460: for(String arg : getPrefixArguments()) {
Line 461: Argument argument = this.arguments.get(arg);
Line 462: help.append(
--
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: 11
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-HasComments: Yes
a***@ovirt.org
2015-05-11 13:19:04 UTC
Permalink
***@ovirt.org has posted comments on this change.

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


Patch Set 12:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-engine-3.5', 'ovirt-engine-3.4', 'ovirt-engine-3.3', 'ovirt-engine-3.5.2', 'ovirt-engine-3.4.0', 'ovirt-engine-3.3.4', 'ovirt-engine-3.3.3', 'ovirt-engine-3.3.2', 'ovirt-engine-3.3.1'])
--
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 <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-HasComments: No
Alon Bar-Lev
2015-05-11 13:44:23 UTC
Permalink
Alon Bar-Lev has posted comments on this change.

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


Patch Set 12: Code-Review+1
--
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 <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-HasComments: No
Alon Bar-Lev
2015-05-11 13:49:28 UTC
Permalink
Alon Bar-Lev has posted comments on this change.

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


Patch Set 12:

(1 comment)

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 205: * Return map of validated and parsed arguments
Line 206: *
Line 207: * @return Map of validated and parsed arguments
Line 208: */
Line 209: public Map<String, Object> getParsedArgs() {
BTW: changing the variable name should not have changed the interface... but ok, if this name makes sense, although I liked the getArguments().
Line 210: return Collections.unmodifiableMap(parsedArgs);
Line 211: }
Line 212:
Line 213: /**
--
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 <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-HasComments: Yes
m***@redhat.com
2015-05-11 14:39:08 UTC
Permalink
Martin Peřina has posted comments on this change.

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


Patch Set 12:

(9 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;
I would prefer destinationType or valueType instead of convert as it much clearly describes the meaning.
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 {
I would prefer ValueRequirement instead of Type as it much more clearly describes what it is about
Line 20: HAS_ARGUMENT,
Line 21: OPTIONAL_ARGUMENT,
Line 22: NO_ARGUMENT;
Line 23:


Line 20: HAS_ARGUMENT,
Line 21: OPTIONAL_ARGUMENT,
Line 22: NO_ARGUMENT;
Line 23:
Line 24: public static Type valueOfIgnoreCase(String name) {
Why not reuse existing?

public static Type valueOfIgnoreCase(String name) {
if (name != null) {
return valueOf(name.toUpperCase());
}
throw new IllegalArgumentException("Invalid value null");
}
Line 25: for (Type type : values()) {
Line 26: if (name.equalsIgnoreCase(type.name())) {
Line 27: return type;
Line 28: }


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<>();
Please always initialize instance attributes in constructor
Line 157:
Line 158: /**
Line 159: * Set of mandatory arguments to check if user specified all of them.
Line 160: */


Line 157:
Line 158: /**
Line 159: * Set of mandatory arguments to check if user specified all of them.
Line 160: */
Line 161: private Set<String> mandatory = new HashSet<>();
Please always initialize instance attributes in constructor
Line 162:
Line 163: /**
Line 164: * Map of correctly parsed and converted arguments
Line 165: */


Line 162:
Line 163: /**
Line 164: * Map of correctly parsed and converted arguments
Line 165: */
Line 166: private Map<String, Object> parsedArgs = null;
Please remove null assignment, it's the default
Line 167:
Line 168: /**
Line 169: * List of errors which was found during pasring
Line 170: */


Line 167:
Line 168: /**
Line 169: * List of errors which was found during pasring
Line 170: */
Line 171: private List<Throwable> errors = null;
Please remove null assignment, it's the default
Line 172:
Line 173: /**
Line 174: * Inititilize ArgumentsParser attributes. Parser properties file and create argument map of it.
Line 175: *


Line 221: public boolean parse(List<String> args) {
Line 222: parsedArgs = new HashMap<>();
Line 223: errors = new ArrayList<>();
Line 224:
Line 225: while(!args.isEmpty()) {
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 226: String arg = args.get(0);
Line 227: if(!arg.startsWith(LONG_PREFIX)) {
Line 228: break;
Line 229: }


Line 246: value == null &&
Line 247: (
Line 248: argument.getType() == Argument.Type.OPTIONAL_ARGUMENT ||
Line 249: argument.getType() == Argument.Type.HAS_ARGUMENT
Line 250: )
I would still prefer this condition:

if (value == null
&& argument.getType() != Argument.Type.NO_ARGUMENT) {

IMO it's much more readable
Line 251: ) {
Line 252: if(args.size() > 0) {
Line 253: value = args.get(0);
Line 254: if (value.startsWith(LONG_PREFIX)) {
--
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 <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-HasComments: Yes
Alon Bar-Lev
2015-05-11 14:43:09 UTC
Permalink
Alon Bar-Lev 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/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<>();
Post by m***@redhat.com
Please always initialize instance attributes in constructor
why?
Line 157:
Line 158: /**
Line 159: * Set of mandatory arguments to check if user specified all of them.
Line 160: */


Line 221: public boolean parse(List<String> args) {
Line 222: parsedArgs = new HashMap<>();
Line 223: errors = new ArrayList<>();
Line 224:
Line 225: while(!args.isEmpty()) {
Post by m***@redhat.com
Usually it's a bad behavior to modify Collection provided by caller. Wouldn
no, as the other arguments are left for farther parsing.
Line 226: String arg = args.get(0);
Line 227: if(!arg.startsWith(LONG_PREFIX)) {
Line 228: break;
Line 229: }


Line 246: value == null &&
Line 247: (
Line 248: argument.getType() == Argument.Type.OPTIONAL_ARGUMENT ||
Line 249: argument.getType() == Argument.Type.HAS_ARGUMENT
Line 250: )
it is not as it is not obvious what other values there are.
Line 251: ) {
Line 252: if(args.size() > 0) {
Line 253: value = args.get(0);
Line 254: if (value.startsWith(LONG_PREFIX)) {
--
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 <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-HasComments: Yes
Alon Bar-Lev
2015-05-11 14:51:40 UTC
Permalink
Alon Bar-Lev has posted comments on this change.

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


Patch Set 12:

(2 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;
Post by m***@redhat.com
I would prefer destinationType or valueType instead of convert as it much c
but this is a conversion from string to a type, it is a conversion process.
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 {
Post by m***@redhat.com
I would prefer ValueRequirement instead of Type as it much more clearly des
Can be HasArg per getopt_long or in properties has_arg, although the existing properties structure is pretty clear when this is type.

has_arg
is: no_argument (or 0) if the option does not take an argument; required_argument (or 1) if the option requires an argument; or optional_argument (or 2) if
the option takes an optional argument.
Line 20: HAS_ARGUMENT,
Line 21: OPTIONAL_ARGUMENT,
Line 22: NO_ARGUMENT;
Line 23:
--
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 <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-HasComments: Yes
m***@redhat.com
2015-05-11 15:01:38 UTC
Permalink
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;
Post by Alon Bar-Lev
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 {
Post by Alon Bar-Lev
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<>();
Post by Alon Bar-Lev
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 <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-HasComments: Yes
Alon Bar-Lev
2015-05-11 15:06:04 UTC
Permalink
Alon Bar-Lev has posted comments on this change.

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


Patch Set 12:

(2 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 15: private String metavar;
Line 16: private boolean multivalue;
Line 17: private String value;
Line 18:
Line 19: enum Type {
Post by m***@redhat.com
Sorry, but HasArg IMO doesn't describe the enum any better than Type.
for these who understand getopt it is while you trying to take it to somewhere else.
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<>();
Post by m***@redhat.com
It's good behavior and you don't look up through the whole file what's the
it was good behavior in java-1.0 or java-2.0 where it was impossible.

constructor should initialize only members that their value are result of constructor parameters.
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 <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-HasComments: Yes
m***@redhat.com
2015-05-11 15:32:14 UTC
Permalink
Martin Peřina has posted comments on this change.

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


Patch Set 12:

(2 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 15: private String metavar;
Line 16: private boolean multivalue;
Line 17: private String value;
Line 18:
Line 19: enum Type {
Post by Alon Bar-Lev
for these who understand getopt it is while you trying to take it to somewh
Sorry, I don't care about getopt here (yes, I used it in the past in C, so I know what you are referring to).I want engine code to be readable and understandable
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<>();
Post by Alon Bar-Lev
it was good behavior in java-1.0 or java-2.0 where it was impossible.
I don't see anything great on in-place initialization, it just make the the code less readable (even though Java compiler moves in-place initialization into constructor code in byte code).
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 <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-HasComments: Yes
Alon Bar-Lev
2015-05-11 15:55:55 UTC
Permalink
Alon Bar-Lev has posted comments on this change.

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


Patch Set 12:

(1 comment)

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<>();
Post by m***@redhat.com
I don't see anything great on in-place initialization, it just make the the
the great in-place is that constructors are left with only these initializations that requires logic. all common initialization are common to all constructors and at variable definition. there is no reason to make constructors complex.
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 <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-HasComments: Yes
a***@ovirt.org
2015-05-11 17:45:06 UTC
Permalink
***@ovirt.org has posted comments on this change.

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


Patch Set 13:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-engine-3.5', 'ovirt-engine-3.4', 'ovirt-engine-3.3', 'ovirt-engine-3.5.2', 'ovirt-engine-3.4.0', 'ovirt-engine-3.3.4', 'ovirt-engine-3.3.3', 'ovirt-engine-3.3.2', 'ovirt-engine-3.3.1'])
--
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: 13
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-HasComments: No
Alon Bar-Lev
2015-05-11 17:47:46 UTC
Permalink
Alon Bar-Lev has posted comments on this change.

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


Patch Set 13:

(1 comment)

one last note regarding last change

https://gerrit.ovirt.org/#/c/40157/13/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 435: );
Line 436: try {
Line 437: argument.setValueType(
Line 438: Class.forName(
Line 439: getArgAttrValue(arg, "convert"),
this should also be changed to valueType in order to be consistent.
Line 440: true,
Line 441: Thread.currentThread().getContextClassLoader()
Line 442: )
Line 443: );
--
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: 13
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-HasComments: Yes
m***@redhat.com
2015-05-12 07:03:17 UTC
Permalink
Martin Peřina has posted comments on this change.

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


Patch Set 13:

(1 comment)

https://gerrit.ovirt.org/#/c/40157/13/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 1: arg.mandatory = false
Line 2: arg.help = no help for this argument
Line 3: arg.type = no_argument
Line 4: arg.convert = java.lang.String
IMO this also needs to be changed to value_type/valuetype/valueType (not sure about conventions here)
Line 5: arg.matcher = .*
Line 6: arg.metavar = STRING
Line 7: arg.multivalue = false
Line 8: #arg.value =
--
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: 13
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-HasComments: Yes
Alon Bar-Lev
2015-05-12 07:05:38 UTC
Permalink
Alon Bar-Lev has posted comments on this change.

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


Patch Set 13:

(1 comment)

https://gerrit.ovirt.org/#/c/40157/13/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 1: arg.mandatory = false
Line 2: arg.help = no help for this argument
Line 3: arg.type = no_argument
Line 4: arg.convert = java.lang.String
Post by m***@redhat.com
IMO this also needs to be changed to value_type/valuetype/valueType (not su
yes, wrote that comment in source.
Line 5: arg.matcher = .*
Line 6: arg.metavar = STRING
Line 7: arg.multivalue = false
Line 8: #arg.value =
--
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: 13
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-HasComments: Yes
a***@ovirt.org
2015-05-12 07:21:49 UTC
Permalink
***@ovirt.org has posted comments on this change.

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


Patch Set 14:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-engine-3.5', 'ovirt-engine-3.4', 'ovirt-engine-3.3', 'ovirt-engine-3.5.2', 'ovirt-engine-3.4.0', 'ovirt-engine-3.3.4', 'ovirt-engine-3.3.3', 'ovirt-engine-3.3.2', 'ovirt-engine-3.3.1'])
--
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: 14
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-HasComments: No
Alon Bar-Lev
2015-05-12 07:52:29 UTC
Permalink
Alon Bar-Lev has posted comments on this change.

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


Patch Set 14: Code-Review+1
--
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: 14
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-HasComments: No
m***@redhat.com
2015-05-12 09:29:39 UTC
Permalink
Martin Peřina has posted comments on this change.

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


Patch Set 14: Code-Review+1
--
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: 14
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Peřina <***@redhat.com>
Gerrit-Reviewer: Mooli Tayer <***@redhat.com>
Gerrit-Reviewer: Ondra Machacek <***@redhat.com>
Gerrit-Reviewer: Oved Ourfali <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: mooli tayer <***@redhat.com>
Gerrit-HasComments: No
Continue reading on narkive:
Loading...