remkop / picocli

Picocli is a modern framework for building powerful, user-friendly, GraalVM-enabled command line apps with ease. It supports colors, autocompletion, subcommands, and more. In 1 source file so apps can include as source & avoid adding a dependency. Written in Java, usable from Groovy, Kotlin, Scala, etc.
https://picocli.info
Apache License 2.0
4.92k stars 425 forks source link

Ability to standardize command names/aliases #1799

Open rsenden opened 2 years ago

rsenden commented 2 years ago

Our picocli application has a structure similar to the following:

root
 |
 - entity1
    |
    - upload
    - download
    - create
    - list|ls
    - delete|rm|remove
 - entity2
    |
    - create
    - list|ls
    - delete|rm|remove
 - entity3
    ...

To ensure a consistent command hierarchy, we want to standardize the command names, aliases and possibly other command attributes.

For command names, we can easily create a constants class and then use for example:

@Command(name=StandardEntityCommands.DELETE)

Unfortunately, this is not possible for aliases as you cannot refer to array constants from annotations; the following doesn't work:

@Command(name=StandardEntityCommands.DELETE, aliases=StandardEntityCommands.DELETE_ALIASES)

You could do something like the following, but then you won't be able to easily add or remove aliases:

@Command(name=StandardEntityCommands.DELETE, aliases={StandardEntityCommands.DELETE_ALIAS1, StandardEntityCommands.DELETE_ALIAS2})

I think the best way to accomplish this is to allow for creating custom Command annotations that specify default values for the various @Command attributes, i.e.

@Retention(RetentionPolicy.RUNTIME)
@Target({ElementType.TYPE})
@Command(name="delete", aliases={"rm", "remove"})
public @interface DeleteCommand {}

@DeleteCommand
public class Entity1DeleteCommand {
   ...
}

@DeleteCommand @Command(subcommands={...}) // Merge command attributes from @DeleteCommand and @Command
public class Entity2DeleteCommand {
   ...
}

Any chance something like this could be implemented, properly supporting auto-complete and man-page generation? Any other way to reuse command attributes across multiple command implementations?

rsenden commented 2 years ago

Actually, based on our earlier discussion in https://github.com/remkop/picocli/issues/1665, maybe this can already be accomplished using mixins as below?

@Command(name="delete", aliases={"rm", "remove"})
public class DeleteCommandMixin {}

public class Entity1DeleteCommand {
   @Mixin DeleteCommandMixin deleteCommandMixin;
   ...
}

public class Entity2DeleteCommand {
   @Mixin DeleteCommandMixin deleteCommandMixin;
   ...
}

Edit: Doesn't seem to work; seems like picocli requires an @Command annotation with a name attribute directly on the subcommand class. Maybe it will work for specifying additional command attributes like aliases, but the resulting code wouldn't look very good (if it works at all, haven't tested this yet):

@Command(aliases={"rm", "remove"})
public class DeleteCommandAliasesMixin {}

@Command(name=StandardEntityCommands.DELETE)
public class Entity1DeleteCommand {
   @Mixin DeleteCommandAliasesMixin deleteCommandAliasesMixin;
   ...
}
remkop commented 2 years ago

Yes, that last example is how I intended mixins to be used. I did not anticipate command names to be reusable, I assumed that command names would be unique. Maybe that was a mistake, I am not sure. I suspect it does make sense for the majority of use cases.

What would be required to add support for custom Command annotations?

rsenden commented 2 years ago

@remkop Just has a look at this, I think this should be relatively simple:

I'm just not sure whether CommandReflection::extractCommandSpec is also used for things like auto-complete and man page generation; if they have their own methods for processing Command annotations, then that logic will need to be updated as well, possibly making things more complex.

rsenden commented 2 years ago

Alternatively, we could look into the ability to also specify command name through mixins, and not requiring a Command annotation with name attribute on command classes. Not sure though whether that's easy to implement, and I think custom command annotations look better (no need to define a mixin field that is not otherwise used).

rsenden commented 2 years ago

Just had a further look at this, looks like auto-complete and man page generators just create a new CommandLine object that indirectly uses CommandReflection::extractCommandSpec, so updating that method as described in my earlier comment would also cover those use cases.

However, annotation processors use their own methods for processing Command annotations, so AbstractCommandSpecProcessor::buildCommands and related methods would need to be updated as well. I'm not familiar with implementing annotation processors though, so not sure how this class would need to be updated to add support for Command-annotated annotations.

rsenden commented 2 years ago

@remkop Any suggestions on how to best implement this feature? This would really be of big benefit to our CLI application to ensure consistent command names and aliases. For example, we already have 23 list commands, with more coming, and a similar amount of update, delete, create, ... commands.

For now, best we can do is something like the following, which is cumbersome and easy to forget the mixin:

@Command(name=StdCmds.LIST)
public class SomeListCommand {
    @Mixin StdCmds.LIST_ALIASES listAliases; // This mixin would have a Command annotation defining the aliases
    ...
}

The ability to define custom annotations annotated with @Command could take some effort to implement, but would result in the cleanest code, i.e. something like the following:

@ListCommand
public class SomeListCommand {
    ...
}

Alternatively, if it's easier to implement, we could remove the requirement to have an @Command annotation with name attribute on command classes, and allow command name to be specified on mixins:

public class SomeListCommand {
    @Mixin ListCommand listCommand; // Both name and aliases defined through Command annotation on mixin
}

Some other alternatives that I've been thinking of, but none of them are really pretty:

remkop commented 2 years ago

I won’t be able to spend much time on this, if any.

I’m not convinced that this is a very common requirement, but perhaps the ability to have custom annotations is generally useful. However if this ability comes at the price of a lot of complexity then I’m not sure if it is a good trade-off…

Any PR to implement this should cover both the runtime reflection and the annotation processor.

remkop commented 1 year ago

Hi @rsenden, can this ticket be closed?

rsenden commented 1 year ago

Hi @remkop, given our current codebase, support for custom annotations probably wouldn't provide much value anymore. However, it would still be useful if command name could be defined through a Mixin.

Our code currently looks something like this, having a separate mixin for each type of command (List, Get, Create, ...), with each mixin providing generic functionality related to that command type:

@Command(name=BasicOutputHelperMixins.List.CMD_NAME)
public class ProxyListCommand extends AbstractBasicOutputCommand implements IRecordTransformerSupplier {
    @Mixin @Getter private BasicOutputHelperMixins.List outputHelper;
    ...
}
public class BasicOutputHelperMixins {
    @ReflectiveAccess @Command(name = "list", aliases = {"ls"})
    public static class List extends AbstractBasicOutputHelper {
        public static final String CMD_NAME = "list";
        @Getter @Setter(onMethod=@__({@Spec(Target.MIXEE)})) private CommandSpec mixee;
        @Getter @Mixin private OutputWriterWithQueryFactoryMixin outputWriterFactory;
        @Getter private StandardOutputConfig basicOutputConfig = StandardOutputConfig.table(); 
    }
    ...
}

Now we need to explicitly have @Command(name=BasicOutputHelperMixins.<Type>.CMD_NAME) on every command; it would be great if the command name could be 'inherited' from the mixin, such that we can have an empty @Command annotation on our command classes.

We already did something similar for 'inheriting' aliases in #1836, however I guess defining and using the command name defined on a Mixin might be a bit more involved.