API change: parse results for subcommands

[remkop]

Problem with Usage Help for Subcommands

Adding support for nested subcommands (#127) highlighted a problem with the CommandLine::parse(String...) method: it currently returns a List with the annotated objects that were registered as subcommands.

This is a problem because without the CommandLine objects associated with these annotated objects, the usage help cannot list the subcommands registered on those CommandLine instances...

Example: suppose we have a subcommand hierarchy like this:

CommandLine commandLine = new CommandLine(new Main())
        .addCommand("cmd1", new Cmd1())
        .addCommand("cmd2", new Cmd2())
        .addCommand("cmd3", new CommandLine(new Cmd3())
                .addCommand("cmd3sub1", new Cmd3Sub1())
                .addCommand("cmd3sub2", new Cmd3Sub2())
        );

Suppose the user asked for help on subcommand cmd3:

<main class> cmd3 --help

In the current picocli API, this is hard to implement since the parsing result does not give a reference to the CommandLine instance that the nested subcommands are registered. It returns a list of annotated objects instead of a list of CommandLine objects:

List<Object> parsed = commandLine.parse(args);
assert parsed.size() == 2;
assert parsed.get(0) instanceof Main;
assert parsed.get(1) instanceof Cmd3;

// This will not show the nested subcommands for "cmd3"
CommandLine.usage(parsed.get(1), System.err);  

The parse result should allow the client code to get a reference to the CommandLine object associated with the parsed subcommand.

[remkop]

Initial thoughts for a solution:

1. return a List<CommandLine> from the parse method.
2. introduce a new custom interface (ParsedCommands?) from which both the annotated objects and the CommandLine objects can be obtained, and return this from the parse method.

Option 1 is simple but a bit cumbersome to use. Client code always needs to call CommandLine::getAnnotatedObject on every element in the list. It may also be surprising to get a list of CommandLine objects when the client code only added annotated objects as commands.

One idea for option 2:

interface ParsedCommands {
    List<Object> annotatedObjects();
    List<CommandLine> commandLines();
}

[pditommaso]

What if the CommandLine retains a map of all parsed objects, and then the usage could be applied with an instance method instead of a static one, eg:

List<Object> parsed = commandLine.parse(args);
assert parsed.size() == 2;
assert parsed.get(0) instanceof Main;
assert parsed.get(1) instanceof Cmd3;

// This will not show the nested subcommands for "cmd3"
commandLine.usage(parsed.get(1), System.err); 

[remkop]

Thought about it some more and I'm now leaning towards simply returning a List<CommandLine>. Combined with issue #135 you raised about the clashing method name, the new signature would become:

/**
 * <p>
 * Initializes the annotated object that this {@code CommandLine} was constructed with as well
 * as possibly any registered commands, based on the specified command line arguments,
 * and returns a list of all commands that were initialized by this method.
 * </p>
 *
 * @param args the command line arguments to parse
 * @return a list with all commands and subcommands initialized by this method
 * @throws ParameterException if the specified command line arguments are invalid
 */
public List<CommandLine> parseCommands(String... args) {...}

Showing usage help then becomes very easy:

List<CommandLine> parsed = commandLine.parseCommands(args);
assert parsed.size() == 2;
assert parsed.get(0).getAnnotatedObject() instanceof Main; // slightly longer than it is now
assert parsed.get(1).getAnnotatedObject() instanceof Cmd3;

// Show the nested subcommands for "cmd3"
parsed.get(1).usage(System.err);  // intuitively obvious for client code to do the right thing

Generally I prefer returning the full information in the result to storing some temporary information in the CommandLine instance: conceivably the same CommandLine instance could be used to parse multiple command line arg arrays, but that may not work correctly if the top-most CommandLine instance still has stored some results from previous invocations.

[pditommaso]

Fair enough. Just a comment of method name, I found getAnnotatedObject a bit cryptic, maybe getCommandObject(or even getCommand) will made the code more readable.

[remkop]

I hadn't realized, thanks for pointing that out. I like getCommand, it is short and maps naturally with the @Command annotation.

There is some potential confusion with addCommand and getCommands. Perhaps these should be renamed to addSubcommand and getSubcommands. Putting it all together:

public                          CommandLine   (Object command)                  {...} // constructor
public Object                   getCommand    ()                                {...} // returns constructor parameter
public CommandLine              addSubcommand (String name, Object subcommand)  {...}
public Map<String, CommandLine> getSubcommands()                                {...}
public static <T> T             parse         (T command, String... args)       {...}
public List<CommandLine>        parseCommands (String... args)                  {...}
public static void              usage         (Object command, PrintStream out) {...}

[pditommaso]

IMO getCommand is coherent with addCommand, I would change instead getCommands to getCommandLines or getCommandsMap.

Just my 2 cents ;)

[remkop]

Sorry, I only just saw your comment.
getCommand returns the object passed into the constructor, and is the top-level command, while addSubcommand and getSubcommands are one level below the top-level command. I kind of like that this difference in level is now explicit. It makes sense to me. Also, the docs have been talking about them as "subcommands" for a while; now the code and the docs are more in sync, which is good.

I think #127 has unearthed a number of hidden deficiencies, I'm quite happy with all these changes.
However, I'm going to let these changes sink in for a bit and keep this in master and not release a new binary just yet.