Work is hosted in annotation-cli-190129 branch started as
http://hg.netbeans.org/core-main/rev/9950a2502fbb

OK, here is new version. I'll polish the versioning, apichanges, etc. and I'd like to integrate on Dec 31, 2011: http://hg.netbeans.org/core-main/rev/04c717040dd9

Javadoc available at http://deadlock.netbeans.org/hudson/job/prototypes-annotation-cli-190129/javadoc/

I've just moved the annotations into org.netbeans.spi.sendopts package. Probably it does not make much sense to have a separate package for three classes that moreover reference the Env class from the org.netbeans.spi.sendopts package.

[JG01] The patch to CLICoreBridge looks like it could introduce a regression in startup performance. Does this load all module layers, or just lib/*.jar and core/*.jar?

The usage of Services/OptionProcessors is also undesirable since we would prefer to deprecate the whole Services/ folder and move as much as possible out of it. (For compatibility reasons, the MIMEResolver and JavaHelp subfolders have to be retained, though in the case of MIMEResolver this folder path is hardcoded - FolderLookup via Lookup.default is not used.)

Consider creating non-layer-based registrations instead, e.g. using META-INF/namedservices/org.netbeans.spi.sendopts.annotations.ProcessArgs/open-group. This would achieve the same lazy-loading effect but without requiring changes to core or requiring the overhead of layer XML parsing. (For use from --help, META-INF/services/org.netbeans.spi.sendopts.annotations.ProcessArgs should also be created so all available processors can be enumerated and @Description found.)


[JG02] ElementType.FIELD for @Messages is covered in a separate review; please do not add it here. It is not even used in the current patch.


[JG03] split("#") should set the limit to 2.

And for consistency with all other localizable annotation attributes, it should be supported to use an unlocalized string (no '#').


[JG04] Typo: additionaArgs


[JG05] Why is longName() not default ""?


[JG06] What is the purpose of falling back to ServiceLoader? Surely it can be expected that the mandatory dependencies of this module, including Lookup, are on the classpath even in a standalone app.

(In reply to comment #5)
> Consider creating non-layer-based registrations instead

FWIW, a complete working demo that this is way easier using SezPoz:

public interface RequiredArgHandler {
    void process(Env env, String value);
}
@Target({ElementType.TYPE, ElementType.METHOD, ElementType.FIELD})
@Retention(RetentionPolicy.SOURCE)
@Indexable(type=RequiredArgHandler.class)
public @interface RequiredArg {
    char shortName();
    String longName();
}
@ServiceProvider(service=OptionProcessor.class)
public class Handler extends OptionProcessor {
    private final Map<Option,IndexItem<RequiredArg,RequiredArgHandler>> items = new WeakHashMap<Option,IndexItem<RequiredArg,RequiredArgHandler>>();
    @Override protected Set<Option> getOptions() {
        Set<Option> opts = new HashSet<Option>();
        for (IndexItem<RequiredArg,RequiredArgHandler> item : Index.load(RequiredArg.class, RequiredArgHandler.class)) {
            Option opt = Option.requiredArgument(item.annotation().shortName(), item.annotation().longName());
            opts.add(opt);
            items.put(opt, item);
        }
        return opts;
    }
    @Override protected void process(Env env, Map<Option,String[]> maps) throws CommandException {
        for (Map.Entry<Option,String[]> pair : maps.entrySet()) {
            try {
                items.get(pair.getKey()).instance().process(env, pair.getValue()[0]);
            } catch (InstantiationException x) {
                throw new CommandException(2, x.toString());
            }
        }
    }
}
public class Example {
    @RequiredArg(shortName='o', longName="open")
    public static final RequiredArgHandler open = new RequiredArgHandler() {
        @Override public void process(Env env, String value) {
            env.getOutputStream().println("opened " + value);
        }
    };
    @RequiredArg(shortName='c', longName="close")
    public static final RequiredArgHandler close = new RequiredArgHandler() {
        @Override public void process(Env env, String value) {
            env.getOutputStream().println("closed " + value);
        }
    };
    private Example() {}
}

Thanks for the review.

(In reply to comment #5)
> [JG01] The patch to CLICoreBridge looks like it could introduce a regression in
> startup performance. Does this load all module layers, or just lib/*.jar and
> core/*.jar?

It needs to load all module layers, as utilities and projectui are provided by regular modules. Obviously, this is not going to make anything faster, but as far as I can tell, the slowdown is not that big (it seems the slowest operation is I/O to open all JARs).

> The usage of Services/OptionProcessors is also undesirable since we would
> prefer to deprecate the whole Services/ folder and move as much as possible out
> of it.

Well, I am not aware of any decision to deprecate the Services folder. The declarative registrations (compared to original direct usage of implementation classes) seem to scale reasonably well. I'd like to stick with layer based registrations.

> [JG02] ElementType.FIELD for @Messages is covered in a separate review; please
> do not add it here. It is not even used in the current patch.

As soon as I merge most recent version of default branch, the change will be gone.

> [JG03] split("#") should set the limit to 2.

fd936eee5875

> And for consistency with all other localizable annotation attributes, it should
> be supported to use an unlocalized string (no '#').

af4d99067ddf

> [JG04] Typo: additionaArgs

dc5aeabe546d

> [JG05] Why is longName() not default ""?

I used to have default for longName, but it is sort of weird to have an annotation that has all optional attributes, yet it cannot be used without specifying at least one of them. After second thought I came to conclusion that majority of CLI options should have a long form, so I made that attribute mandatory (with an option to specify "").

> [JG06] What is the purpose of falling back to ServiceLoader? Surely it can be
> expected that the mandatory dependencies of this module, including Lookup, are
> on the classpath even in a standalone app.

Right mandatory dependencies need to be on classpath. However in order to compete with https://args4j.dev.java.net/ and http://commons.apache.org/cli/, I am trying to lower the amount of necessary dependencies. Thus Lookup is not mandatory, especially when using CommandLine.create(...).

(In reply to comment #6)
> (In reply to comment #5)
> > Consider creating non-layer-based registrations instead
> 
> FWIW, a complete working demo that this is way easier using SezPoz:

I don't think this is a complete solution, but yes, it is well known that LayerGeneratingProcessor is more verbose than other existing solutions. Btw. here is new diff: http://hg.netbeans.org/core-main/rev/b0b03c1f4df4

Still on track to merge on Dec 31, 2011.

Merged as core-main#44f8c20e9260