Alon Bar-Lev has posted comments on this change. Change subject: tools: Fix command line args in engine-manage-domains ......................................................................
Patch Set 1: (4 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: } > Well, I thinks it redundant, since you have always enter some actions. So I this is kind of a violation of args, but I can live with it. 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)); > Well, ExtendedCliParser is my code, but then you have to set index to 0 whe add two methods, like write() and such. Line 198: } catch (IllegalArgumentException ex) { Line 199: throw new ManageDomainsResult(ManageDomainsResultEnum.INVALID_ARGUMENT, ex.getMessage()); Line 200: } Line 201: } else { 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; > IMO printing help is not an exception it is a result... better throw than return in middle. in this case i am not considering it as help at all... it is an error ('please specify action') 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, > Well, moving values seems to me easier and more readable then constructing I think that metadata programming is superior... you create the metadata for what you need, then iterate over the metadata. the metadata is more clear then the logic. give it a chance. args = new Args() args.addArg(ACTION_EDIT, ARG_PROVIDER) args.addArg(ACTION_EDIT, ARG_PROVIDER, false) ... this is only as java cannot create maps on the fly... I thought of making it simpler with some json/xml parser if possible... as embedded string or as a resource: { 'edit': { 'provider': true, 'xxxx': false, }, ... } 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
