Martin Peřina has posted comments on this change. Change subject: tools: Fix command line args in engine-manage-domains ......................................................................
Patch Set 1: (5 comments) http://gerrit.ovirt.org/#/c/23842/1/backend/manager/tools/src/main/java/org/ovirt/engine/core/domains/ManageDomainsArguments.java File backend/manager/tools/src/main/java/org/ovirt/engine/core/domains/ManageDomainsArguments.java: Line 183: } Line 184: Line 185: if (!isValidAction(args[0])) { Line 186: throw new ManageDomainsResult(ManageDomainsResultEnum.INVALID_ACTION, args[0]); Line 187: } > I suggest --action= to make it consistent not go based on positional. Well, I thinks it redundant, since you have always enter some actions. So I thought the git style would be better ... Line 188: Line 189: String action = args[0]; Line 190: argMap.put(ARG_ACTION, action); Line 191: Line 193: if (args.length > 1) { Line 194: // entered more args then just action Line 195: try { Line 196: ExtendedCliParser parser = new ExtendedCliParser(); Line 197: parsed = parser.parse(Arrays.copyOfRange(args, 1, args.length)); > why not provide parser with start index and length? if not our code, then o Well, ExtendedCliParser is my code, but then you have to set index to 0 when you want to parse whole array (Java doesn't have parameters with default value yet) Line 198: } catch (IllegalArgumentException ex) { Line 199: throw new ManageDomainsResult(ManageDomainsResultEnum.INVALID_ARGUMENT, ex.getMessage()); Line 200: } Line 201: } else { Line 198: } catch (IllegalArgumentException ex) { Line 199: throw new ManageDomainsResult(ManageDomainsResultEnum.INVALID_ARGUMENT, ex.getMessage()); Line 200: } Line 201: } else { Line 202: parsed = Collections.<String, String> emptyMap(); > why not set it to empty map and avoid conditional? True, I will change this. Line 203: } Line 204: Line 205: if (!parsed.isEmpty()) { Line 206: if (parsed.containsKey(ARG_HELP)) { Line 205: if (!parsed.isEmpty()) { Line 206: if (parsed.containsKey(ARG_HELP)) { Line 207: // print help Line 208: argMap.put(ARG_HELP, null); Line 209: return; > raise ManageDomainResult? IMO printing help is not an exception Line 210: } Line 211: Line 212: // config file can be entered for all actions Line 213: moveArgsValues(parsed, ARG_CONFIG_FILE); Line 221: checkRequiredArgsValue(ARG_DOMAIN, ARG_PROVIDER, ARG_USER, ARG_PASSWORD_FILE, Line 222: ARG_CHANGE_PASSWORD_URL); Line 223: Line 224: } else if (ACTION_EDIT.equals(action)) { Line 225: moveArgsValues(parsed, > why do we need move? Well, moving values seems to me easier and more readable then constructing Map<String, Map<String, Boolean>> and evaluating it. And moving valid args away is also easier to throw an error if some unneeded/unknown args were entered (from line 254) Line 226: ARG_DOMAIN, ARG_PROVIDER, ARG_USER, ARG_PASSWORD_FILE, ARG_LDAP_SERVERS, Line 227: ARG_ADD_PERMISSIONS, ARG_CHANGE_PASSWORD_URL); Line 228: checkRequiredArgs(ARG_DOMAIN); Line 229: checkRequiredArgsValue(ARG_DOMAIN, ARG_PROVIDER, ARG_USER, ARG_PASSWORD_FILE, -- To view, visit http://gerrit.ovirt.org/23842 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1296958a54d8b802e0d6fb59f1f75cc608e75818 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Peřina <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: Juan Hernandez <[email protected]> Gerrit-Reviewer: Martin Peřina <[email protected]> Gerrit-Reviewer: Yair Zaslavsky <[email protected]> Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
