Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ability to standardize command names/aliases #1799

Open
rsenden opened this issue Sep 6, 2022 · 9 comments
Open

Ability to standardize command names/aliases #1799

rsenden opened this issue Sep 6, 2022 · 9 comments

Comments

@rsenden
Copy link
Contributor

rsenden commented Sep 6, 2022

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
Copy link
Contributor Author

rsenden commented Sep 6, 2022

Actually, based on our earlier discussion in #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
Copy link
Owner

remkop commented Sep 7, 2022

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
Copy link
Contributor Author

rsenden commented Sep 7, 2022

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

  • Add ElementType.ANNOTATION as a Target on the Command annotation
  • Update the logic in CommandReflection::extractCommandSpec:
    • Instead of calling method/cls::getAnnotation(Command.class), call method/cls::getAnnotations()
    • Iterate over all annotations to find all Command annotations, either specified directly or on another annotation
    • If multiple Command annotations are found, either throw an exception, or implement logic for merging attributes from multiple annotations (append where appropriate, like aliases, otherwise throw an exception if for example 'name' is specified on multiple Command annotations on a single method/class)

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
Copy link
Contributor Author

rsenden commented Sep 7, 2022

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
Copy link
Contributor Author

rsenden commented Sep 12, 2022

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
Copy link
Contributor Author

rsenden commented Sep 15, 2022

@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:

  • Add a Class<? extends Supplier<String[]>> aliasSupplier() attribute to Command, so you can do something like @Command(name=StdCmds.LIST, aliasSupplier=StdCmds.LIST_ALIASES.class)
  • Use something like @Command(name=StdCmds.LIST, aliases={StdCmds.LIST_ALIAS1, StdCmds.LIST_Alias2}, but not sure whether picocli allows for empty/null aliases in case there's only one alias (or no alias at all); you'd want to reference a fixed number of alias constants in the command implementations, such that you don't have to update all command implementations when adding or removing an alias.

@remkop
Copy link
Owner

remkop commented Sep 16, 2022

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
Copy link
Owner

remkop commented Dec 23, 2022

Hi @rsenden, can this ticket be closed?

@rsenden
Copy link
Contributor Author

rsenden commented Dec 23, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants