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

Reply via email to