Skip to content

Commit

Permalink
deprecated the old methods
Browse files Browse the repository at this point in the history 
redirecting uses to new methods in registry
  • Loading branch information
Stephan Fuhrmann committed Jul 4, 2014
1 parent 6f675b0 commit b1aa90f
Showing 1 changed file with 6 additions and 2 deletions.
8 changes: 6 additions & 2 deletions args4j/src/org/kohsuke/args4j/CmdLineParser.java
View file
Expand Up @@ -110,7 +110,8 @@ public void addArgument(Setter setter, Argument a) {
Utilities.checkNonNull(setter, "Setter");
Utilities.checkNonNull(a, "Argument");

OptionHandler h = createOptionHandler(new OptionDef(a,setter.isMultiValued()),setter);
OptionHandler h = OptionHandlerRegistry.getRegistry().createOptionHandler(this,
new OptionDef(a,setter.isMultiValued()),setter);

This comment has been minimized.

Sign in to view

Copy link
@davido

davido May 11, 2015

Contributor

Was it really necessary to deprecate createOptionHandler(OptionDef o, Setter setter) method and not call it from addArgument() and addOption() methods? I understand the refactoring, done in 304dcb2. But this part of this change prevent developers from easily supplying their own instances of option and argument handlers.

Note that in non trivial cases OptionHandlerRegistry machinery cannot be used. Why? Because IoC thing. When dependency injection framework is used, the custom option handlers must be part of the injection graph. Once this is the case ctors of option handlers can be passed much more arguments than the default ctor, and thus such instances cannot be created by OptionHandlerRegistry and only can be created by derived class from CmdLineParser (using Google Guice assisted inject feature).

But this change makes this behavior unnecessary complicated, because the overriding of createOptionHandler() doesn't have any effect any more and OptionHandlerRegistry is used directly from addArgument() and addOption() methods. The only way to supply home baked instances of handlers is now to override the addArgument() and addOption() methods, like suggested in this change: [1].

But I think this is to much noise to do something that simple as supply custom created instances of option handlers. That why I think that this part of change has serious drawback for this specific use case, while giving us no advantage and should be reverted.

[1] https://gerrit-review.googlesource.com/67793
int index = a.index();
// make sure the argument will fit in the list
while (index >= arguments.size()) {
Expand Down Expand Up @@ -138,7 +139,8 @@ public void addOption(Setter setter, Option o) {
for (String alias : o.aliases()) {
checkOptionNotInMap(alias);
}
options.add(createOptionHandler(new NamedOptionDef(o), setter));
options.add(OptionHandlerRegistry.getRegistry().createOptionHandler(
this, new NamedOptionDef(o), setter));
}

/**
Expand Down Expand Up @@ -166,6 +168,7 @@ private void checkOptionNotInMap(String name) throws IllegalAnnotationError {
/**
* Creates an {@link OptionHandler} that handles the given {@link Option} annotation
* and the {@link Setter} instance.
* @deprecated You should use {@link OptionHandlerRegistry#createOptionHandler(org.kohsuke.args4j.CmdLineParser, org.kohsuke.args4j.OptionDef, org.kohsuke.args4j.spi.Setter) } instead.
*/
protected OptionHandler createOptionHandler(OptionDef o, Setter setter) {
Utilities.checkNonNull(o, "OptionDef");
Expand Down Expand Up @@ -695,6 +698,7 @@ protected boolean isOption(String arg) {
* {@link OptionHandler#OptionHandler(CmdLineParser, OptionDef, Setter)}
* @throws NullPointerException if {@code valueType} or {@code handlerClass} is {@code null}.
* @throws IllegalArgumentException if {@code handlerClass} is not a subtype of {@code OptionHandler}.
* @deprecated You should use {@link OptionHandlerRegistry#registerHandler(java.lang.Class, java.lang.Class)} instead.
*/
public static void registerHandler( Class valueType, Class<? extends OptionHandler> handlerClass ) {
Utilities.checkNonNull(valueType, "valueType");
Expand Down

0 comments on commit b1aa90f

Please sign in to comment.