-
Notifications
You must be signed in to change notification settings - Fork 420
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
Provide convenience API for global options (was: Feature request: inheriting mixins in subcommands) #649
Comments
Picocli provides a The user manual has an example. |
Thanks, but I'm not sure how I would use it. In my Where would I put the Are you saying I could put this in the annotation for
Will that work? (I doubt it.) Even if it does, I don't see the point, because it would seem that Perhaps I'm just not understanding what you're suggesting. |
The picocli-examples module has this example: https://github.com/remkop/picocli/blob/master/picocli-examples/src/main/java/picocli/examples/subcommands/ParentCommandDemo.java |
You could define a For example, if the user invoked:
The This can be implemented something like this: @Command(name = "top-level-command", subcommands = SubCommand.class)
class TopLevelCommand {
@Option(name = "--debug") boolean debug;
}
@Command(name = "subcommand", subcommands = SubSubCommand.class)
class SubCommand {
@Option(name = "--debug") boolean debug;
@ParentCommand
TopLevelCommand topLevelCommand;
}
@Command(name = "sub-subcommand")
class SubSubCommand {
@Option(name = "--debug") boolean debug;
@ParentCommand
SubCommand myParent;
public void run() {
if (this.debug || myParent.debug || myParent.topLevelCommand.debug) {
System.out.println("I'm debugging...");
}
}
} |
Yeah, I'm still not getting it. I have the feeling you might not have read my example very closely. The example that was given uses a static subclass for a command. But in my example the subcommand is a method. Where do I put an annotation to an instance variable inside a method? Methods can't have instance variables. They just have variables on the stack. Let me show all the pieces together: @Command(name = "foo", mixinStandardHelpOptions = true)
public class Foo extends BaseCliApplication {
@Option(names = {"--debug", "-d"}, description = "Turns on debug level logging.")
protected void setDebug(final boolean debug) {
…
}
@Command(description = "Do the bar thing.")
public void bar(…) {
…
} So where would I use Are you saying that I must change my subcommand to use a static internal class? This is starting to look messy: @Command(name = "foo", mixinStandardHelpOptions = true)
public class Foo extends BaseCliApplication {
@Option(names = {"--debug", "-d"}, description = "Turns on debug level logging.")
protected void setDebug(final boolean debug) {
…
}
@Command(name = "bar", description = "Do the bar thing.")
static class Bar implements Runnable {
public void run() {
//I have to transfer the logic from the bar() method to here
}
} And that was just to refactor. Now I have to go back and add the @Command(name = "foo", mixinStandardHelpOptions = true)
public class Foo extends BaseCliApplication {
@Option(names = {"--debug", "-d"}, description = "Turns on debug level logging.")
protected void setDebug(final boolean debug) {
…
}
@Command(name = "bar", description = "Do the bar thing.")
static class Bar implements Runnable {
@ParentCommand
private Foo parent;
public void run() {
//I have to transfer the logic from the bar() method to here
//plus now I have to do this??
if(parent.isDebug()) {
//wait, what do I do here? if parent.setDebug() had been called, I wouldn't need any of this
}
}
} I guess I'm still lost. |
No, but that's the point—the If I add the The whole point of creating |
Ah ok, yes, I missed that the Methods do not inherit from How about using a mixin? class DebugMixin {
@Option(names = "--debug")
protected void setDebug(final boolean debug) {
// one idea is to update static state here, so that the DebugMixin instance does not matter
…
}
}
@Command(name = "foo", mixinStandardHelpOptions = true)
public class Foo extends BaseCliApplication {
@Mixin
DebugMixin debugMixin;
@Command(description = "Do the bar thing.")
public void bar(@Mixin DebugMixin debugMixin, @Option other…) {
…
} Would that work? |
OK, so I have good news and bad news. If I manually add @Command(name = "foo", mixinStandardHelpOptions = true)
public class Foo extends BaseCliApplication {
@Option(names = {"--debug", "-d"}, description = "Turns on debug level logging.")
protected void setDebug(final boolean debug) {
…
}
@Command(description = "Do the bar thing.")
public void bar(@Option(names = {"--debug", "-d"} boolean debug) {
…
} The bad news is that this defeats half the purpose of having a base application class. Now I'll have to remember to include So this will get me by in the short term, but we really need an option that says @Command(name = "foo", mixinStandardHelpOptions = true)
public class Foo extends BaseCliApplication {
@Option(names = {"--debug", "-d"}, description = "Turns on debug level logging.", inherited=true)
protected void setDebug(final boolean debug) {
…
}
@Command(description = "Do the bar thing.")
public void bar() {
…
} That way, any subcommand of any subclass of Would it be agreeable to add such an (BTW I see our replies are sort of going past each other as we both respond. 😄 ) To respond to your And the weird thing is, if I add this So for the meantime I'll just add |
I'm not sure what you mean by
Picocli should only call parent command Foo.setDebug() if the user specified Are you sure this is what is happening? Can you share code that reproduces this behaviour? Generally, Picocli offers two mechanisms for reuse: inheritance and mixins. Command methods cannot inherit, so that leaves mixins. For a single option, it may be too much overhead to define a mixin class, I guess that depends on the implementation of the Mixins allow you to invoke logic when an If you do decide to go with a mixin, I like to make mixins reusable components, so it would probably make more sense to move the logic from |
Oops, you're right. The reason it worked for me is that I had added a hack workaround to get debug turned on by calling 😞
But even if I refactor the code to switch to internal classes (see above), that doesn't help anything. So this "inheritance" doesn't help my situation, regardless of whether it's for a subcommand or not. Mixins are bulky and, and require that I remember in every single application and every single command to add this mixin manually. We need a simply way to say, "this option applies to all subcommands". It's a simple flag. Why do we need to make things complicated? I think this mixin idea is wonderful for more complicated needs that require more flexibility and more access. But this is not a complicated need. It is a simple need. |
So, given an option defined in a command (which is bound to a field or method in that command) you’re asking to introduce a mechanism for adding this option to subcommands, while keeping these subcommand options bound to the parent command field or method. I need to think about this. At first glance it seems very application-specific and I have doubts that it is a general enough use case to warrant support in the library. But I could be wrong. I frequently am. For now I would recommend using a Mixin to allow invoking the |
Definitely, and so do I! My proposal was meant to be the impetus for more thinking and coming up with the best design. The initial solution was off the top of my head; we may still think up something better, or realize why this is a bad idea. On the surface of it, though, it would seem something generally useful. I'll be thinking more about it too, thanks. |
I don't see how this would work for options that are bound to method parameters (when the option is defined on an annotated public class Xxx {
@Command(subcommands = Baz.class)
public void bar(@Option(names = "--debug", inherited=true) boolean debug) {
…
}
@Command(name = "baz")
static class Baz implements Runnable {
public void run() {
// how can baz find out whether `bar baz --debug` was specified?
}
}
} |
First let me ask you this (because I haven't used this configuration): in the example you gave, if I issue the command |
Your library allows a lot of combinations of things that can work together. We can't use all the combinations together at one time, because they don't always make sense. You already pointed out, for example, that So the simple answer to your question seems to be that you can say that the "inherited" flag only works for annotations on accessor methods (e.g. That seems like a reasonable answer and not too much of a kludge. Nobody expects all the combinations of features to work with all the available ways there is to put the application together. |
By default, only |
I try to avoid providing features that only sometimes work. One could argue that I would only consider introducing a potentially confusing feature like this if it brings considerable benefits when it is applicable. Given that the same (and more) can be achieved with mixins, the only argument for this new mechanism is that it gives more terse syntax than mixins. To be honest, I am liking this less the more I think about it. |
But we've only thought about it for an hour or so! haha I don't even know how much I like it either. I think about things for weeks or longer. So far I'm liking the idea of a mixin that can be declared "inherited". It is self-contained. If a subcommand doesn't need the information passed, then it doesn't need to redeclare the mixin. The mixin would still "do its job" down the hierarchy as long as it was declared "inherited". I could do It seems that what I'm talking about are options for cross-cutting concerns—things that apply across functionalities. These include:
So I would want to turn on debugging for any command. I would want to set the user interface language for any command. I might want to specify the configuration for any command. But there is still more thinking to do. |
To put this into perspective, for the past several years I've created a series of small, loosely-coupled libraries for cross-cutting concerns. (They are refactorings of functionality I've built into applications for over a decade.) These include: Take logging for instance. I have a base application class as I mentioned, I'm still not sure that's the perfect path forward, but hopefully you understand better that this isn't some esoteric, one-off functionality that's needed for some niche application. |
The thing is, all If you look at picocli's mechanisms for reuse, it is clear how the binding works for both, and how the command can retrieve the specified values:
Any new mechanism reuse would need to clearly define how the bindings for the reused options and positional parameters would work. With java class inheritance and mixins, it is very easy to understand for users how this works. People can just use the annotations without thinking about how things work under the hood. I doubt we will be able to achieve a similar simplicity with an additional reuse mechanism. I really hesitate to introduce a 3rd mechanism for reuse. Bear in mind, if you don't like using mixins, perhaps instead of using |
Could you tell me more about how to do this? My applications already inherit from
Well, if Picocli already has a mechanism for reuse, then I'll use that! So here is what I want to know: what can I add to |
Basically what you were doing originally, but using classes for commands instead of If @Option(names = {"--debug", "-d"}, description = "Turns on debug level logging.")
protected void setDebug(final boolean debug) {
// I don't know what this does but I assume it modifies static state
// so that it works the same regardless of whether it is set
// from a subcommand or from a top-level command
} And all your commands inherit (directly or indirectly) from For example: @Command(name = "foo", subcommands = Bar.class)
public class Foo extends BaseCliApplication implements Runnable {
@Option(name = "--other")
int other;
// ...
}
@Command(name = "bar", subcommands = Baz.class)
public class Bar extends BaseCliApplication implements Callable<Void> {
@Option(name = "--other-option")
String otherOption;
public Void call() throws Exception {
// ...
}
@Command(name = "baz")
static class Baz extends BaseCliApplication implements Callable<Void> {
@Option(name = "--yet-other-option")
String yetAnother;
public Void call() throws Exception {
// ...
}
}
} |
But
Maybe I could refactor the Even if that works (I'll have to try it), still that pollutes the structure of the application a lot. It was so simple, and I was so happy, to be able to add a subcommand just by adding a new method |
@Command(name = "bar", subcommands = Baz.class)
public class Bar extends BaseCliApplication implements Callable<Void> {
@Option(name = "--other-option")
String otherOption;
public Void call() throws Exception {
// ...
} But look how complicate this has become! Before I was able to do this: @Command(name = "bar")
void bar(@Option(name = "--other-option") String option) {
// ...
}
} |
I was just trying to answer your question, sorry to upset you. We've gone over the options that are available to you now for defining a |
I'm not upset. I was just listing the downsides of the current inheritance approaches, because you seemed to imply the existing functionality was sufficient.
And that's why I was proposing a solution that would meet these needs. But you don't want to allow any additional functional here. And it is your library, of course. But since it's such a big need, and it applies to so many cross-cutting functionality configurations, I'll likely just keep limping along with Picocli until I get some time and then just write my own system (or maybe just expand on the one I wrote 10 or 15 years ago). Thanks for the discussion. |
@garretwilson Reopening as we may be able to accomplish this without introducing a completely new mechanism for reuse. Let's look further at your idea to add an attribute to the mixin annotation that tells picocli to apply the mixin to subcommands. So it could be used something like this:
This raises some new questions:
This last point may be addressed with an explicit exclusion list:
The first point is trickier. |
I'm cleaning up my inbox and wanted to circle back on this ticket. I hope you are are doing well, and that you are keeping safe and healthy in this world health crisis.
Perhaps, but in your example I question the terms "local" and "global". If I set Hopefully within the next month or two I'll be back to the point on my project where I can upgrade to the latest picocli version, so I news on this from time to time is great. |
In the latest version (now in master) it’s actually called |
Quick update: I am hoping to do a release that includes this feature soon. Relevant snippet from the release notes follows below: Inherited OptionsThis release adds support for "inherited" options. Options defined with Below is an example where an inherited option is used to configure logging. @Command(name = "app", subcommands = Sub.class)
class App implements Runnable {
private static Logger logger = LogManager.getLogger(App.class);
@Option(names = "-x", scope = ScopeType.LOCAL) // option is not shared: this is the default
int x;
@Option(names = "-v", scope = ScopeType.INHERIT) // option is shared with subcommands, sub-subcommands, etc
public void setVerbose(boolean verbose) {
// Configure log4j.
// This is a simplistic example: you probably only want to modify the ConsoleAppender level.
Configurator.setRootLevel(verbose ? Level.DEBUG : Level.INFO);
}
public void run() {
logger.debug("-x={}", x);
}
}
@Command(name = "sub")
class Sub implements Runnable {
private static Logger logger = LogManager.getLogger(Sub.class);
@Option(names = "-y")
int y;
public void run() {
logger.debug("-y={}", y);
}
} Users can specify the
Specifying the
|
Good morning! I guess this isn't released yet? I just finished off a some big features in my main project, so for a break I figured I would come bring in some of the exciting new picocli feature you've been talking about. So this morning I brought up my project and did a search, but Maven Central is still showing v4.2.0 so I guess the version you mentioned is still in the oven. Oh, well. I guess I'll work on something else this morning. I'm looking forward to the new version, though. Have a good week. |
Getting close but not yet. I also took on some extra work helping out in the https://github.com/petermr/openVirus project which reduced the time I could spend on picocli. |
Thanks for the update. There's no emergency need for it at the moment. I'll circle back in a week or two. Good luck. |
Picocli 4.3 has been released. Enjoy! |
As luck would have it I hit a huge milestone a couple of days ago in one of my projects, so today I actually took a day off from my employer, intending to spend the morning in tranquility integrating the new changes. (Right away I ran into a new Eclipse regression bug so I should have known it wouldn't be that easy.) Here is my original base Picocli application debug setting method, as you've no doubt seen many times: /**
* Enables or disables debug mode, which is disabled by default.
* @param debug The new state of debug mode.
*/
@Option(names = {"--debug", "-d"}, description = "Turns on debug level logging.")
protected void setDebug(final boolean debug) {
this.debug = debug;
updateLogLevel();
} So with Picocli 4.3 I thought all I needed to do was to add an "inherit" scope: /**
* Enables or disables debug mode, which is disabled by default.
* @param debug The new state of debug mode.
*/
@Option(names = {"--debug", "-d"}, description = "Turns on debug level logging.", scope = ScopeType.INHERIT)
protected void setDebug(final boolean debug) {
this.debug = debug;
updateLogLevel();
} In CLI subclass I had this: @Command(description = "Does foo and bar.", subcommands = {HelpCommand.class})
public void foobar(
@Parameters(paramLabel = "<project>", description = "The base directory of the project being served.%nDefaults to the working directory, currently @|bold ${DEFAULT-VALUE}|@.", defaultValue = "${sys:user.dir}", arity = "0..1") @Nullable Path argProjectDirectory,
…
@Option(names = {"--debug", "-d"}, description = "Turns on debug level logging.") final boolean debug) throws IOException, LifecycleException {
setDebug(debug); //TODO inherit from base class; see https://github.com/remkop/picocli/issues/649 So I removed the @Command(description = "Does foo and bar.", subcommands = {HelpCommand.class})
public void foobar(
@Parameters(paramLabel = "<project>", description = "The base directory of the project being served.%nDefaults to the working directory, currently @|bold ${DEFAULT-VALUE}|@.", defaultValue = "${sys:user.dir}", arity = "0..1") @Nullable Path argProjectDirectory,
…
) throws IOException, LifecycleException { Two little changes, and I should be good to go. But no, I get:
I don't know where it's coming from. I don't know which arguments. But interestingly if I actually add the
The stack trace makes me suspect that your code is calling the subcommand method and, since one of the options was inherited, the code thinks that it should be an argument to the subcommand method (here But surely you tested this, so I don't know how this could happen. In any case, I'm going to suspend work on this for the moment and try to get some tranquility this morning. Maybe I'll try to integrate the new injected Picocli |
Away from PC now, will look into this soon. |
I raised #1042 This is a bug. Thank you for raising this! |
@remkop I've integrated this into my application, and so far it is everything I dreamed it would be! Admittedly from the outside it doesn't look like a lot technically: it was just a flag to say "inherit this option to subcommands". But I hope you realize how much nicer it makes my application by simply adding that simple This is really useful; thank you so much. |
Thanks for that! What would be really great is if you could do a blog post or article somewhere on how picocli has been useful for you. You’d be surprised how few people know that picocli even exists. Would be good if we could change that. |
I would absolutely love to! I haven't been producing blog entries for a while because I'm been working feverishly to migrate my entire personal site to a new Cloud-distributed platform.
But the new site was just launched, and soon the static site generator and metadata framework will be ready for the next version release. Then I would be enthusiastic to spread the word! If you don't hear anything further from me on this in about a month, by all means don't hesitate to ping me on this. |
@remkop @garretwilson Do you have an example of where you used this? As I get If I have something like: The class of SubCommand extends the class of the Command (base class). How do I make Option1 and Option2 available when the order of arguments is specified as above |
Why are we getting a DuplicateOption exception?@palade I am guessing that the When the application uses 2 reuse mechanisms (see sidebar below), the option is added to the subcommand twice... I will look at improving the error message to clarify this. Sidebar: Picocli now offers 3 methods of reuse:
Are options ignored sometimes?
To summarize my understanding: this part of the question is not about using As a result, now the top-level command has Option1 and Option2 and When you say "Option1 and Option2 are ignored", I am guessing you mean that the How to solve this?So, now we understand what is happening, what shall we do to fix it? Idea 1: make Option1 and Option2 "inherited options"Two ways to do this:
This is especially useful if Option1 and Option2 are "standalone" options (like for configuring logging) that subcommands don't need to reference. If a subcommand do need to reference inherited options, it can use the @Command(mixinStandardHelpOptions = true, versionProvider = XX.class)
class BaseCommand {
}
@Command(name = "top", subcommands = SubCommand.class)
class TopCommand extends BaseCommand implements Runnable {
@Option(name = "Option1", scope = INHERIT) boolean option1;
@Option(name = "Option2", scope = INHERIT) boolean option2;
public void run() {
// business logic of top-level cmd...
}
}
@Command(name = "sub")
class SubCommand extends BaseCommand implements Runnable {
@ParentCommand TopCommand parent;
public void run() {
if (parent.option1) { doOption1Stuff(); }
if (parent.option2) { doOption2Stuff(); }
if (this.option3) { doOption3Stuff(); }
}
} Idea 2: Use the
|
@palade , I think I remember getting this error at first. My main command is the class That wasn't what I was expected. I had hoped that by using Here is the ticket I used to perform the conversion. It should (in the "Development" section) link to a couple of commits and a pull request that should show you clearly the changes I made: https://globalmentor.atlassian.net/browse/JAVA-195 Oh, actually that is just changing the debug command to an inherited command in the base class. In the subclass CLI application, I had to remove the debug option as a parameter. That was done in this ticket: https://globalmentor.atlassian.net/browse/GUISE-114 I think these are the important commits:
Hope this helps. |
Thank you both for your replies. The Option1 and Option2 in my case are still ignored. These are logging info options which could be placed anywhere in the command-line arguments. Please note that the parse handler is run with the RunLast option. The sub-commands are added programmatically to the top command, and to each is passed the entire array of args. What I don't understand is why the position of these options are important and how to include them in the sub-commands. |
I see this post on SO that RunLast will invoke only the last command. Still wondering if there is a way to pass the Option1 and Option2 to the sub-command. Otherwise if it is difficult or not possible, would be a way to adjust the USAGE, because at the moment shows like: topcommand [Option1] [Option2] [COMMANDS] would be possible to adjust the USAGE to show the exclusive option: topcommand ( [OPTION1] [OPTION2] | [COMMANDS]) |
@palade Can you create a new ticket, and in the description show a way for me to reproduce the problem? Either example code or a link to a public repository would work. |
@remkop Will attempt to modify the usage message as making the above changes would require a larger codebase change. Was wondering if there some examples where I could programatically create the following customSynopsys:
There are some examples in the manuals, but would like to create this programmtically. I had a look at the help test but it seems that the examples that I found were harding the usage message. Would be possible to create this programmatically (i.e., instead of using the |
@palade I’d be happy to help you accomplish your use case. I believe it would be useful to create a separate ticket: this ticket ticket is closed, already has 70+ comments, and your use case may result in changes to picocli, which would be easier to track in a separate ticket. So please indulge me and raise a separate ticket. Now, I’ve been making guesses about what your code looks like, and I would like to stop guessing. :-) When you mention “my Option1 and Option2 are still being ignored” I have no idea what you tried or what could be the cause. Please help me help you, and take the time to explain
So far I partially understand points 2 and 3 only. |
One more thing: there’s no need to create a custom synopsis We can make it work so that users can specify these options either before or after the subcommand. But please create a separate ticket first with your code, so we can discuss more concretely. |
I made a base class
BaseCliApplication
that all my CLI applications will extend. The base class does all the special picocli setup needed and other things for CLI applications. The class adds a--debug
option that turns on debug-level logging:Unfortunately in my
Foo
CLI application subclass, I introduce a subcommandbar
using a method:Picocli doesn't seem to pick up the
--debug
switch in the subcommand. If I attempt to use the commandfoo bar
, it tells me:Is there a
@Command
parameter that I can use to say "this switch applies to all subcommands"? Or is there a way to specify for a subcommand that the--debug
switch should work for it as well?I looked for documentation for mixins, but I only found information on how to include them if they are already defined. I'm not sure how to define a mixin. How can I say that the
setDebug()
method is a mixin? (Frankly I'd rather use the first approach: some way to say "this switch applies to all subcommands".)Thanks in advance.
The text was updated successfully, but these errors were encountered: