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

Command aliases on Mixin not being applied #1836

Closed
rsenden opened this issue Oct 4, 2022 · 2 comments · Fixed by #1841
Closed

Command aliases on Mixin not being applied #1836

rsenden opened this issue Oct 4, 2022 · 2 comments · Fixed by #1841

Comments

@rsenden
Copy link
Contributor

rsenden commented Oct 4, 2022

Our discussions in #1665 and #1799 seem to suggest that it should be possible to define command aliases through a Mixin, i.e. something like the following:

@Command(aliases = {"ls"} )
public class ListAliasMixin {}

@Command(name="list")
public class MyListCommand {
    @Mixin private ListAliasMixin aliasMixin; 
}

However, for some reason the aliases are not being applied to MyListCommand. Any idea why?

@remkop
Copy link
Owner

remkop commented Oct 6, 2022

Yes looks like aliases are not picked up from mixins. I see this in the code for CommandSpec (see last line):

            public CommandSpec addMixin(String name, CommandSpec mixin) {
                mixins.put(interpolator.interpolate(name), mixin);

                initName(interpolator.interpolateCommandName(mixin.name()));
                // TODO initAliases(mixin.aliases()); // should we?

We can change this. Will you be able to provide a unit test?

@remkop remkop added this to the 4.7 milestone Oct 6, 2022
@rsenden
Copy link
Contributor Author

rsenden commented Oct 6, 2022

I'll look into providing a PR with fix and unit test either myself or by my colleague.

Interestingly, the initName line just above seems to indicate that it should be possible to also initialize the command name through a Mixin, however I don't see how this can actually be used as CommandReflection::subcommandName explicitly checks whether a subcommand has a Command annotation with name attribute.

Most of our commands now look as follows, with MyOutputHelperMixins providing Mixin classes that provide a CMD_NAME constant, possibly command aliases (once this issue is fixed), and other generic functionality:

@Command(name = MyOutputHelperMixins.List.CMD_NAME)
public class SomeListCommand extends AbstractOutputCommand implements IBaseHttpRequestSupplier {
    @Getter @Mixin private MyOutputHelperMixins.List outputHelper;
    ...
}

If we could specify the command name on the List Mixin, we could get rid of @Command(name = MyOutputHelperMixins.List.CMD_NAME) on every command, either having an empty @Command annotation or no @Command annotation at all. This would simplify our code, but more importantly, would avoid mismatches like the following (using the command name from Get, but using the List mixin):

@Command(name = MyOutputHelperMixins.Get.CMD_NAME)
public class SomeListCommand extends AbstractOutputCommand implements IBaseHttpRequestSupplier {
    @Getter @Mixin private MyOutputHelperMixins.List outputHelper;
    ...
}

Any suggestions on how to allow the command name to be specified through a Mixin? Note that by allowing both command name and aliases to be specified through a Mixin, we can also close #1799.

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

Successfully merging a pull request may close this issue.

2 participants