Alon Bar-Lev has posted comments on this change. Change subject: extensions test tool: logger ......................................................................
Patch Set 8: (4 comments) good! looks very good! https://gerrit.ovirt.org/#/c/37886/8/backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/core/ExtensionsToolExecutor.java File backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/core/ExtensionsToolExecutor.java: Line 54: } Line 55: moduleService.parseArguments(others); Line 56: if (moduleService.doHelp()) { Line 57: System.out.println(module + " help"); // TODO: help printer Line 58: return; please avoid return at middle of function. maybe it suggest you have your own exception for normal termination? :) or... condtional. Line 59: } Line 60: Line 61: loadExtensions(moduleService, argMap); Line 62: moduleService.run(); https://gerrit.ovirt.org/#/c/37886/8/backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/core/ModuleService.java File backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/core/ModuleService.java: Line 7: import java.util.Map; Line 8: Line 9: public interface ModuleService { Line 10: Line 11: public static final ExtKey EXTENSIONS_MAP = new ExtKey("GLOBAL_EXTENSIONS_MAP", Map/*<ExtMap>*/.class, "78680401-40cf-4bae-a6ee-7e8c204a9010"); hmmm... I am not well up to date with java conventions... but having data within interface is very strange :) fine by me though :) Line 12: Line 13: public String getName(); Line 14: Line 15: public String getDescription(); https://gerrit.ovirt.org/#/c/37886/8/backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/logger/services/LoggerServiceImpl.java File backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/logger/services/LoggerServiceImpl.java: Line 63: Line 64: @Override Line 65: public void parseArguments(List<String> argsList) throws Exception { Line 66: args = new ParametersParser( Line 67: Util.loadProperties(getClass(), "arguments.properties") util is a very bad name for something in cli parser, as the flat namespace nature after import does not enable rename when conflict. I suggest you have this loadProperties as static method within ParametersParser, or even have constructor that receives resource and it will load the properties. Line 68: ).parse(argsList); Line 69: } Line 70: Line 71: @Override https://gerrit.ovirt.org/#/c/37886/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 4: arg.__any__.convert = java.lang.String Line 5: arg.__any__.default = Line 6: arg.__any__.matcher = .* Line 7: arg.__any__.metavar = STRING Line 8: arg.__any__.multivalue = false new line please.... -- To view, visit https://gerrit.ovirt.org/37886 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie06113c5d56a49e58d557c851f9ff00b9a9ca409 Gerrit-PatchSet: 8 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ondřej Macháček <machacek.on...@gmail.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Ondra Machacek <omach...@redhat.com> Gerrit-Reviewer: Ondřej Macháček <machacek.on...@gmail.com> Gerrit-Reviewer: automat...@ovirt.org 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
