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 { > 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, > 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) { > 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); > The true is that it will lighten access to these errros: 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()); > 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()); > 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 <omach...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Martin Peřina <mper...@redhat.com> Gerrit-Reviewer: Mooli Tayer <mta...@redhat.com> Gerrit-Reviewer: Ondra Machacek <omach...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: mooli tayer <mta...@redhat.com> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
