Sub-subcommands (nested commands)

[pditommaso]

Quick question: Is it possible to define a subcommand of a subcommand ?

[remkop]

@pditommaso Apologies I misread your question.

You can register multiple subcommands, and picocli will recognize command line arguments containing multiple subcommands, but picocli currently does not impose a hierarchy on subcommands.

So, it is not possible to have a subcommand XYZ that is only recognized after previously a "parent" subcommand ABC was recognized. Example:

CommandLine commandLine = new CommandLine(new TopLevel())
        .addCommand("abc",   new ABC())
        .addCommand("def",   new DEF())
        .addCommand("xyz",   new XYZ());

String[] args = {"-global", "def", "-defOption", "xyz", "-xyzOption" };
List<Object> recognized = commandLine.parse(args);

assert recognized.get(0) instanceof TopLevel.class;
assert recognized.get(1) instanceof DEF.class;
assert recognized.get(2) instanceof XYZ.class;

In your application you can now choose to throw an exception if XYZ is only valid after an ABC subcommand. Does that work for you?

[pditommaso]

It could be a workaround but it's not exactly what I'm trying to do. I have a use case in which a command can have subcommands and in some cases there could be sub-subcommands with the same name but different semantic, thus I would need to keep the implementation logic separated.

[remkop]

Ok this sounds doable. Parsing and usage help-wise the current code will need only small modifications.

Still thinking about what the API for registering nested subcommands should look like. I like the method chaining that the current API affords, meaning that the addCommand method returns the outermost CommandLine instance. This is likely the main use case, and a new API for nested subcommands should not make the main use case less convenient...

One idea is to make the first parameter of addCommand a vararg that constitutes the nested subcommand path. For example:

CommandLine commandLine = new CommandLine(new TopLevel())
        .addCommand("abc",               new ABC()) // first level subcommand
        .addCommand("abc", "xyz",        new XYZ()) // xyz is nested under abc (2nd level)
        .addCommand("abc", "xyz", "ZZ",  new ZZ())  // ZZ is nested under xyz (3rd level)
        .addCommand("def",               new DEF()) // first level subcommand
        ;

What do you think? Is it easy to see intuitively what the above code does?

[pditommaso]

Yes, this sounds cool!

[remkop]

...or should there be a separate method?

CommandLine commandLine = new CommandLine(new TopLevel())
        .addCommand      ("abc",               new ABC()) // first level subcommand
        .addNestedCommand("abc", "xyz",        new XYZ()) // xyz is nested under abc (2nd level)
        .addNestedCommand("abc", "xyz", "ZZ",  new ZZ())  // ZZ is nested under xyz (3rd level)
        .addCommand      ("def",               new DEF()) // first level subcommand
        ;

A separate method raises the question of what signature it should have. And I just realized that neither of these will work because a vararg must be the last parameter. :-) Oops.

[remkop]

Ok, so the above won't work because the varargs must come last.

So now I'm thinking of something like the below, but I don't like the ambiguity of "abc", "xyz": it's unclear which one is the ancestor of the other...

CommandLine commandLine = new CommandLine(new TopLevel())
        .addCommand("abc", new ABC())               // first level subcommand
        .addCommand("xyz", new XYZ(), "abc")        // xyz is nested under abc (2nd level)
        .addCommand("ZZ",  new ZZ(),  "abc", "xyz") // ZZ is nested under xyz (3rd level)
        .addCommand("def", new DEF())               // first level subcommand
        ;

[pditommaso]

I tend to prefer the previous syntax but I understand the limitation of open arrays. What it the sub-subcommands are specified by using a String[] as first parameter?

The more elegant solution would be to add sub-command directly on a command eg.

new ABC() .addCommand("xyz", new XYZ())

But it would require commands to implement a command abstract class or even better an interface with a default method (which implies only Java 8).

[remkop]

Yes, there should be no need to implement any interfaces or do anything other than the normal annotations for nested subcommands. Ease of use is one of the drivers of this project, so where possible I like to provide a fluent API to allow method call chaining.

One idea is to wrap commands in a CommandLine instance when you want to add nested commands to it:

CommandLine sub2SubC = new CommandLine(new SubSub2C())
        .addCommand("sub2-subC-aaa", new SubSubSub2CA())
        .addCommand("sub2-subC-bbb", new SubSubSub2CB());

CommandLine sub2 = new CommandLine(new Sub2())
        .addCommand("sub2-subA", new SubSub2A())
        .addCommand("sub2-subB", new SubSub2B())
        .addCommand("sub2-subC", sub2SubC); // register the previously created hierarchy as a command
                    
CommandLine topLevel = new CommandLine(new MyCommand())
        .addCommand("sub1", new Sub1())
        .addCommand("sub2", sub2)           // register a 2-level hierarchy as a command
        .addCommand("sub3", new Sub3())

The good thing is that this way there is no ambiguity about which is a subcommand of which. The disadvantage is this may be a bit confusing because you need to create the most deeply nested command first.

The hierarchy is more obvious when written in a fluent way like this:

CommandLine commandLine = new CommandLine(new MyCommand())
            .addCommand("sub1", new Sub1())
            .addCommand("sub2", new CommandLine(new Sub2())
                    .addCommand("sub2-subA", new SubSub2A())
                    .addCommand("sub2-subB", new SubSub2B())
                    .addCommand("sub2-subC", new CommandLine(new SubSub2C())
                            .addCommand("sub2-subC-aaa", new SubSubSub2CA())
                            .addCommand("sub2-subC-bbb", new SubSubSub2CB())
                    )
            )
            .addCommand("sub3", new Sub3())
            .registerConverter(Path.class, s -> Paths.get(s))
            .registerConverter(Duration.class, s -> Duration.parse(s));

[pditommaso]

I like this approach, this would allow each command to provide its own graph of subcommands, in a very OOP way.

[remkop]

@pditommaso I just pushed this to master. Can you verify? If you can contribute tests that would be great.
I plan to add more tests for Help with nested subcommands.
Also still need to update the user manual.

[remkop]

Regarding custom type converters, the common use case is likely that they should apply to all nested subcommands. However, it should be possible to have a custom type converter apply only to one of the subcommands (or not apply to one of the subcommands).

To achieve this, custom type converters are applied to the full subcommand hierarchy as it exists when the type converter is registered. Subcommands added later won't automatically have this type converter. Updated the user manual and javadoc to explain the above.

[pditommaso]

I've added a test for sub-subcommands, but I can't manage to make it work. This test returns 1 instead of 3. I guess I make some trivial mistake but I can manage to figure out what's the problem.

One more thing: Do you know Spock framework? Would it be fine for you if I use it for contributing tests?

[remkop]

@pditommaso Yes Spock is fine.
The reason the test returns 1 is that it was given a single string "cloud create". This is a single argument, not an array of Strings. What you wanted to do is:

String[] args = {"cloud", "create"};
List result = cli.parse(args);

I have made this exact same mistake several times myself...

[pditommaso]

Oops, you are right. But this bring a new question: should not an exception raised when an command/option is specified ?

[remkop]

Probably yes. I was thinking along these lines: #132. Feedback very welcome!

[pditommaso]

Great last thing, how is your imports IDE configuration? I've noticed that my IntelliJ IDEA configuration is completely reorganising all imports.

[remkop]

Exporting my IntelliJ IDEA 2017.1.2 Editor > Codestyle > Java > scheme gives this:

<code_scheme name="Default (1)">
  <option name="GENERATE_FINAL_LOCALS" value="true" />
  <option name="GENERATE_FINAL_PARAMETERS" value="true" />
  <option name="CLASS_COUNT_TO_USE_IMPORT_ON_DEMAND" value="99" />
  <option name="NAMES_COUNT_TO_USE_IMPORT_ON_DEMAND" value="1" />
  <option name="JD_ALIGN_PARAM_COMMENTS" value="false" />
  <option name="JD_ALIGN_EXCEPTION_COMMENTS" value="false" />
  <option name="WRAP_COMMENTS" value="true" />
  <XML>
    <option name="XML_LEGACY_SETTINGS_IMPORTED" value="true" />
  </XML>
  <codeStyleSettings language="JAVA">
    <option name="CALL_PARAMETERS_WRAP" value="1" />
    <option name="METHOD_PARAMETERS_WRAP" value="5" />
    <option name="RESOURCE_LIST_WRAP" value="1" />
    <option name="EXTENDS_LIST_WRAP" value="1" />
    <option name="THROWS_LIST_WRAP" value="1" />
    <option name="EXTENDS_KEYWORD_WRAP" value="1" />
    <option name="THROWS_KEYWORD_WRAP" value="1" />
    <option name="METHOD_CALL_CHAIN_WRAP" value="1" />
    <option name="BINARY_OPERATION_WRAP" value="1" />
    <option name="TERNARY_OPERATION_WRAP" value="1" />
    <option name="ARRAY_INITIALIZER_WRAP" value="1" />
    <option name="ASSIGNMENT_WRAP" value="1" />
    <option name="ASSERT_STATEMENT_WRAP" value="1" />
    <option name="IF_BRACE_FORCE" value="3" />
    <option name="DOWHILE_BRACE_FORCE" value="3" />
    <option name="WHILE_BRACE_FORCE" value="3" />
    <option name="FOR_BRACE_FORCE" value="3" />
    <option name="ENUM_CONSTANTS_WRAP" value="2" />
  </codeStyleSettings>
</code_scheme>

[remkop]

@pditommaso Just checking in, how are things going with the tests? I may do a release soon (in the next few days) after implementing the remaining tickets to improve validation (#132 and #138). We can always add the tests later if you prefer but I can hold off for a little bit and include them in the release.

[pditommaso]

Sorry, I'm very busy this week and I couldn't progress on tests. I will try to catch up in the weekend.

[remkop]

Closing this ticket since the functionality has been implemented and version 0.9.7 has been release.
Please submit a pull request with tests at your convenience.
Thanks for raising this!