Probably should switch this thread to commons-dev@, are you subscribed there?

Andrew Ferguson wrote:
Thanks for that, I've added a new test (based on yours) and fixed

existing ones. It turns out that GroupImpl was only validating options that are present and skipping those that were missing. > I've fixed this in cvs but no binary is available yet.

ok thanks, I've updated my local checkout now and picked it up. I've
being playing with it some more and have some more minor points..

1) On the Builder classes the reset() method has a void return type


This should be unecessary since create() causes an implicit reset(). On the other hand, returning 'this' doesn't do any harm and will be liked by the paranoid among us. Done.



2) Some error messages could be more specific eg in DefaultOption.validate

Good catch - will deal with this ASAP.


3) (As noted before) in the "svn/cvs"-style usage there is no easy way to determine which command has been invoked - a (hacky?) workaround is to do something like

        String command = null;
        for(Iterator i = line.getOptions().iterator(); i.hasNext(); ) {
                Option o = (Option) i.next();
                if(!o.getPreferredName().startsWith("-")) {
                        if(command!=null) {
                                throw new RuntimeException("You can only
specify one command"); // this is never thrown as the top-level Group
has a maximum of 1
                        } else {
                                command = o.getPreferredName();
                        }
                }
        }

        I'm not sure what a nice API for this would be though - maybe
something to look at only the top level options passed (and not their
children)

                CommandLine line = ...
                ...
                line.getTopLevelOptions(); ??

I suspect that the loop above is the most sensible thing to do, although I'd probably use an "o instanceof Command" clause rather than checking for the prefix. Personally I tend to work the other way around and have a construct along the lines of:


if(line.contains("update")){
    doUpdate(line);
}
else if(line.contains("commit")){
    doCommit(line);
}
else{
    help.print();
}

This way you never really ask the question which option is present. I'm nervous of adding methods such as getTopLevelOptions() as it seems very particular to the SVN style of options and I'm not sure how useful it would be in others (I think that even CVS would need a group of options that contains the group of commands to correctly model it, and how getTopLevelOptions would know that I'm not sure).


4) (As noted before) For non-group option implementations nesting exceptions might be a way of allowing detailed information about where the parse error occurred eg in the validate method in Command you could have

    public void validate(WriteableCommandLine commandLine)
        throws OptionException {
        if (isRequired() && !commandLine.hasOption(this)) {
            throw new OptionException(this);
        }
        try {
                super.validate(commandLine);
        } catch(OptionException oe) {
                OptionException mine = new OptionException(this); // or
maybe the message should be altered to "Command "+preferredName+"
"+oe.getMessage());
                mine.initCause(oe);
                throw mine;
        }
    }

        which would mean the HelpFormatter would need to iterate to the
end of the chain of exceptions to find the true message (or as in the
comment above parent options could prefix additional
        information about what is going on). Its probably better to have
the HelpFormatter construct this detailed message as an option if this
idea has any milage in it

I've been pondering this for the last few days, I think chaining (or otherwise augmenting) exceptions is probably the right thing to do. Essentially the HelpFormatter needs to be able to identify the correct level of context to display. One method would be to tell HelpFormatter that the context is always X options into the chain, another would be to fix at Y options from the end of the chain. Another method for identifying the context would be to add a property to ParentImpl (or maybe OptionImpl) to indicate whether that option should be used as the context, and then search for the last matching option.


I'm pretty sure that we can find a sensible solution and out do cvs/svn's level of helpfulness. On the other hand I'm still mulling over the options here so thoughts are welcome!!


Also, if you do need/want some extra coding/docs time then I might be able to get permission to allocate some time to lending a hand?

Patches are always appreciated, whether in docs, test, code or just ideas! Although such discussions should probably migrate to [EMAIL PROTECTED]


Thanks,

Rob



thanks, Andrew

--------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]



Reply via email to