Support subcommand methods in Groovy scripts

[attiand]

I’m not sure if this is supposed to work, but it would be very cool if it did.

@Grab('info.picocli:picocli-groovy:4.5.1')
@GrabConfig(systemClassLoader = true)
@Command(name = "picocli",
        mixinStandardHelpOptions = true,
        version = '1.0.0',
        subcommands = [ HelpCommand.class ],
        description = 'sub command test')

@picocli.groovy.PicocliScript
import static picocli.CommandLine.*


@Command(description = "Record changes to the repository")
void commit(@Option(names = ["-m", "--message"]) String commitMessage,
            @Option(names = "--squash", paramLabel = "<commit>") String squash,
            @Parameters(paramLabel = "<file>") File[] files) {

    println "commit ${files}"
}

println "done"

The groovy script seems to pick up the sub command in the usage help, but i can’t get the script to call the commit method.

./picocli.groovy --help
Usage: picocli [-hV] [COMMAND]
sub command test
  -h, --help      Show this help message and exit.
  -V, --version   Print version information and exit.
Commands:
  help    Displays help information about the specified command
  commit  Record changes to the repository


./picocli.groovy commit picocli.groovy 
done

[remkop]

Thanks for raising this!
I’ll take a look.

[remkop]

Analysis

Looking at this, I am finding various problems with the PicocliBaseScript implementation... It has basically become a bit out of sync with the main usage patterns emphasized in the user manual (specifically, the execute method). PicocliBaseScript.run duplicates the equivalent of CommandLine.call as of picocli 2.0:

• no support for @Command-annotated methods (this ticket)
• always runs the most deeply nested subcommand and the script body. Yes, both! (And it ignores any IExecutionStrategy)
• no support for help subcommands (either the built-in CommandLine.HelpCommand or custom implementations)
• invalid user input is handled by PicocliBaseScript.handleParameterException, instead of IParameterExceptionHandler. The printed message is different: PicocliBaseScript additionally shows the arguments, but does not show any suggestions for misspelled options/subcommands.
• any runtime exception is handled by PicocliBaseScript.handleExecutionException, instead of IExecutionExceptionHandler. The script code wraps the user exception in a picocli.CommandLine.ExecutionException and rethrows that.
• limited support for exit codes. Returned objects diverge from java picocli:
  • invalid input returns 1 (java picocli returns 2, and this is customizable in the annotations)
  • requests for help and version return null (java picocli returns 0, and this is customizable in the annotations)
  • runtime exceptions don't return, but throw ExecutionException (java picocli returns 1, and this is customizable in the annotations)

So, how to fix this without breaking any scripts that depend on the above behaviour?

Workaround

One idea to fix this, is that scripts override the run method to call CommandLine.execute. This works with picocli 4.0 and later:

// workaround for picocli 4.0 - 4.5.2
@Override
Object run() {
    return getOrCreateCommandLine().execute(getScriptArguments())
}

This works for scripts that do not override any of the PicocliBaseScript methods.

Long-term Solution 1

In a future release of picocli-groovy, I can make the call to execute the default behaviour, and move the existing logic into a legacyRun method. Scripts can then override run to invoke legacyRun to get the old behaviour.

// solution 1: use CommandLine.execute by default
// The below snippet shows how a script can override the `run` method to fall back to the legacy behaviour.
@Override
Object run() {
    return legacyRun()
}

This could potentially break existing scripts that did customization by overriding any of the PicocliBaseScript methods.

Long-term Solution 2

An alternative long-term solution is to document the workaround and recommend all Groovy scripts use it.
We could create a convenience method (perhaps called execute) that invokes getOrCreateCommandLine().execute(getScriptArguments()), so that scripts can use the below:

// solution 2: use the legacy behaviour by default
// The below snippet shows how a script can override the `run` method to invoke CommandLine.execute.
@Override
Object run() {
    return execute() // invokes `getOrCreateCommandLine().execute(getScriptArguments())`
}

This preserves backwards compatibility but puts a burden on future Groovy script authors. The script does not "automatically do the right thing".

Thoughts?

[deining]

I would vote for implementing long term solution 1.
Any idea about the number of current users/programmers that make use of picicli users in Groovy context?

[remkop]

I don't have any idea about the number of users of picocli-groovy for scripting.

Most scripts will work fine with Solution 1, but scripts that have customized behaviour by overriding any PicocliBaseScript methods may see problems because these methods will no longer be invoked.

"hybrid" approach

Yet another idea is a "hybrid" approach where in addition to Solution 1, we install an IParameterExceptionHandler that calls PicocliBaseScript.handleParameterException, and an IExecutionExceptionHandler that calls PicocliBaseScript.handleExecutionException. This preserves some limited backwards compatibility.

Picocli's default parameterExceptionHandler is nicer (suggests typo corrections), and returns a different exit code than PicocliBaseScript.handleParameterException. I imagine future users would prefer to use the default parameterExceptionHandler.

Hybrid approach has limited scope

Even in this hybrid approach, it may not be possible to invoke the following PicocliBaseScript methods:

• runRunnableSubcommand
• parseScriptArguments
• printVersionHelpMessage
• printHelpMessage - only called when the end user specified invalid input, not when the end user requested help (with --help) - so some inconsistency here

These methods will not be called with Solution 1, even in the hybrid approach. So scripts that customized these methods may encounter problems.

The hybrid approach has the risk that may fail to satisfy both new users (who I imagine prefer the default exception handlers), and existing users whose customization is not covered by the hybrid approach.

[remkop]

@PicocliScript2 and PicocliBaseScript2

Yet another idea is to introduce a new @PicocliScript2 annotation with a new PicocliBaseScript2 base class whose run method simply calls getOrCreateCommandLine().execute(getScriptArguments()).

This PicocliBaseScript2 base class does not have any other methods that can be overridden. Scripts that need to customize anything can override getOrCreateCommandLine, and customize the CommandLine object before returning it, very similar to what a Java program would do in the main method.

This does not break any existing scripts, and new scripts can simply use the new annotation. The documentation would need to be updated.

[remkop]

Introducing a new @PicocliScript2 annotation with a new PicocliBaseScript2 base class is probably the way to go.

I made some progress on this and pushed my changes to master. Documentation is still work in progress.
I tried to keep it compatible with @PicocliScript and PicocliBaseScript, but there are some subtle differences that I need to clarify in the docs.

Any review would be great.

[remkop]

@attiand I think the work for this ticket is complete. To summarize:

• new @PicocliScript2 annotation and associated PicocliBaseScript2 script base class
• the old @PicocliScript and PicocliBaseScript classes are now deprecated
• the new implementation now calls CommandLine::execute
• this supports @Command-annotated methods, help subcommands, and exit codes
• scripts can override beforeParseArgs(CommandLine) to change the parser configuration, set custom exception handlers etc.
• scripts can override afterExecution(CommandLine, int, Exception) to call System.exit or return a custom result.

This will be included in the upcoming picocli 4.6 release.
If you have a chance to review, that would be great.

[attiand]

@remkop looks good, works the way I hoped 👍 Thanks.