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