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

Inherited annotation for commands? #660

Closed
devinrsmith opened this issue Apr 8, 2019 · 8 comments
Closed

Inherited annotation for commands? #660

devinrsmith opened this issue Apr 8, 2019 · 8 comments
Labels
status: declined ❌ A suggestion or change that we don't feel we should currently apply type: enhancement ✨
Milestone

Comments

@devinrsmith
Copy link

I've got a couple commands that share a lot of the same structure, but differ in defaults only. I'd like to be able to specify an abstract command, with specific implementations inhering the abstract classes Command annotation. I think this can be accomplished by adding java.lang.annotation.Inherited to Command.

For example:

@Command(name = "mycommand")
abstract class MyCommand {
  abstract String getContext();
}

class MyCommandFoo extends MyCommand {
  String getContext() { return "foo"; }
}

class MyCommandBar extends MyCommand {
  String getContext() { return "bar"; }
}

Would the Inherited Command annotation be possible?

@remkop
Copy link
Owner

remkop commented Apr 9, 2019

For some reason I completely missed java.lang.annotation.Inherited.

However, picocli already walks the class hierarchy looking for annotations (including @Options and @Parameters etc) from the superclass(es) so I believe this is already working. See reuse and subclassing in the manual for details.

To expand on your example:

@Command(name = "mycommand", footer = "Top-level footer")
abstract class MyCommand {
    abstract String getContext();
}

@Command(name = "foo") // if name is omitted the name of this command becomes "mycommand"
class MyCommandFoo extends MyCommand {
    String getContext() { return "foo"; }

    public static void main(String[] args) {
        new CommandLine(new MyCommandFoo()).usage(System.out);
    }
}

@Command(name = "bar", footer = "Sub-sub footer in bar")
class MyCommandBar extends MyCommand {
    String getContext() { return "bar"; }

    public static void main(String[] args) {
        new CommandLine(new MyCommandBar()).usage(System.out);
    }
}

Now, if I run java MyCommandFoo, I see this:

Usage: foo
Top-level footer

While if I run java MyCommandBar, I see this:

Usage: bar
Sub-sub footer in bar

This proves that not only is the @Command annotation effectively inherited, you can selectively override specific attributes (the footer in the above example). If not overridden, the command inherits the attribute value from its superclass. I hope this is what you have in mind, let me know if otherwise.

That said, I will check if adding the java.lang.annotation.Inherited annotation will allow me to get rid of some logic in picocli. :-) Thanks for the pointer!

@devinrsmith
Copy link
Author

When I remove all the @Command annotation from the concrete class (and just have it on base abstract class) I get an error. Maybe it's b/c it's a subcommand?

Exception in thread "main" picocli.CommandLine$InitializationException: Subcommand com.example.FilesSetCommandUnscoped is missing the mandatory @Command annotation with a 'name' attribute

@devinrsmith
Copy link
Author

devinrsmith commented Apr 9, 2019

Here's a working example:

import picocli.CommandLine;
import picocli.CommandLine.Command;

class Example {

  @Command(name = "mycommand", footer = "Top-level footer")
  abstract static class CommonCommand {
    abstract String getContext();
  }

  static class FooSubcommand extends CommonCommand {

    @Override
    String getContext() {
      return "foo";
    }
  }

  static class BarSubcommand extends CommonCommand {

    @Override
    String getContext() {
      return "bar";
    }
  }

  @Command(name = "foo", subcommands = {FooSubcommand.class})
  static class Foo { }

  @Command(name = "bar", subcommands = {BarSubcommand.class})
  static class Bar { }

  @Command(
      name = "top",
      subcommands = {
        Foo.class,
        Bar.class
      }
  )
  static class TopLevel { }

  public static void main(String[] args) {
    new CommandLine(new TopLevel()).usage(System.out);
  }
}

@remkop
Copy link
Owner

remkop commented Apr 9, 2019

Thanks for the example, I can see that the InitializationException no longer happens after adding the @Inherited annotation to @Command.

The existing unit tests also still pass with @Inherited added, so unless some other problem pops up, I will include this in the next release (4.0-alpha-2).

Thanks for raising this!

@remkop
Copy link
Owner

remkop commented Apr 11, 2019

Pushed to master. Thanks for raising this!

@remkop
Copy link
Owner

remkop commented Jun 4, 2019

@devinrsmith, I am afraid I will have to revert this change, since it is causing failures in other scenarios. Specifically, since this change, users can no longer have multiple levels of inheritance. See #716

For example:

@Command(subcommands = CommandLine.HelpCommand.class)
static abstract class AbstractCommand {
}

static abstract class AbstractCommandSubclass extends AbstractCommand {
}

static class ConcreteCommand extends AbstractCommandSubclass {
}

@Test
public void testNoInitializationException() {
    // if Command is @Inherited, the below line throws
    // picocli.CommandLine$InitializationException: 
    // Another subcommand named 'help' already exists for command '<main class>'
    new CommandLine(new ConcreteCommand());
}

The above test fails when Command is @Inherited.
The test passes when I remove the @Inherited annotation from Command.

For now, I will simply revert this change. I have not investigated if it is possible to address #716 in a way that preserves the @Inherited annotation, but I am open to suggestions.

For now, the impact on your application is that subcommands need a @Command annotation with a name attribute. Any other attributes, like footer etc, are inherited.

@remkop remkop reopened this Jun 4, 2019
@remkop
Copy link
Owner

remkop commented Jun 4, 2019

This was reverted, and the @Command annotation is no longer @Inherited from version 4.0-beta-1.

See this test that shows how to make your example work after this change.

@remkop
Copy link
Owner

remkop commented Oct 11, 2019

I haven't heard any feedback, so closing this ticket.
Feel free to comment here or open another one to discuss further.

@remkop remkop closed this as completed Oct 11, 2019
@remkop remkop added the status: declined ❌ A suggestion or change that we don't feel we should currently apply label Oct 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined ❌ A suggestion or change that we don't feel we should currently apply type: enhancement ✨
Projects
None yet
Development

No branches or pull requests

2 participants