-
Notifications
You must be signed in to change notification settings - Fork 425
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
Missing default value in ArgGroup. #1409
Comments
Not really linked to this issue but as 1A, 1B, 2A and 2B are required, I expected that when X is used (1A AND 1B) or (2A or 2B) will be required. |
Can you run -x ANOTHERVALUE again with |
|
@remkop I have been working on this issue. If I resolve it I will open a PR. |
@MadFoal that would be great, thank you! |
Update 1- looks like the default values do get properly stored in argSpec inside the applyGroupDefaults function correctly. There is something about the way it gets merged back into this that is causing an issue. |
Updated 2- "Picocli will not initialize the @ArgGroup-annotated field (and so no default values are applied) if none of the group options is specified on the command line." (https://picocli.info/#_default_values_in_argument_groups) If you do not specify at least one value within the argument group, it will not instantiate the default values, which is the observed behavior for this case. Technically the program is working as advertised. |
@MadFoal based on the debug output, I think there is a problem:
After parsing is done, picocli is trying to apply default values to all unmatched options in the command, where possible. Since the
However, in the debugger I can see that the default values are applied to a different instance of the What I believe is happening is the following: initially, the I need to dig a bit deeper into what would be the best way to fix this, but I do think the current behaviour is incorrect. |
This is proving trickier than I thought... When an options in a subgroup is matched, a new user object (e.g. a For default values there is no match, so this above logic is not invoked.
So while I can see that from @sbernard31's point of view the current defaulting behaviour is unsatisfactory, I am not sure this can be fixed without major rework, which I am currently not eager to do... |
@remkop Okay thank you for that explanation. I thought it was along those lines, however I am significantly less familiar with the code. I noticed early that we were adding the default values but I was struggling with why they are not making it into the correct place. This makes more sense. I had been looking at how to change "applyDefaultValues" to look for the subgroup created by the -x option and add default values if the current value is null. I believe this is what you are describing as "Another idea was to invoke the same logic for applying..." There is another way of fixing this default behavior also less elegant. In the command declaration we hardcode the default values as such: |
@MadFoal I am not sure there is a true fix. The documentation for default values in arg groups states the situation reasonably well. Applications should probably:
|
@remkop This is one way of overcoming the issue. Do you intend to update the documentation for this issue? |
@MadFoal Will you be able to provide a pull request to improve the documentation? |
@remkop I can do that. |
Great, thank you for working on this!
My recommendation to app authors using groups would be to not instantiate groups in their declaration: // don't do this
class Outer {
// this way, applications cannot tell whether an option in the Inner group was specified on the command line
@ArgGroup Inner inner = new Inner();
} but instead to just declare the field, so that it remains // do this instead
class Outer {
@ArgGroup Inner inner; // this field will only be non-null if an option in this group was matched
} The options in the groups can be initialized in their declaration, but should also have a default value in their annotation: class Inner {
// yes, there is some duplication here, unfortunately
@Option(names = "-a", defaultValue = "AAA") String a = "AAA";
@Option(names = "-b", defaultValue = "BBB") String b = "BBB";
} It may be a good idea to add a // MyApp
public void run() {
if (outer == null) { // no options in the group were specified on the command line; apply defaults
outer = new Outer();
}
if (outer.inner == null) { // same for nested sub-groups
outer.inner = new Inner(); // apply the default values of the inner group
}
// remaining business logic...
} |
I'll make these changes and re-submit. |
PR #1463 with the changes you recommended. Let me know if there are more changes that I need to make. |
I understand that by default defaultValue in ArgGroup are not applied.
The behavior is explained at : #876 (comment)
(Note: The default behavior seems surprising to me at first sight but maybe it's justified)
I faced a situation where it seems that this behavior is not respected (but maybe I missed something)
Sorry for the not so short example, I was not able to make it smaller.
The goal is to have this kind of options : X AND ((1A AND 1B) OU (2A OU 2B))
I set default value for X, 2A and 2B.
I share the code :
When I run my command, it outputs value of each options.
E.g if I run it without any options :
Now if I run it just with option -x : myCommand -x ANOTHER_VALUE, I get :
Default value of 2A and 2B are lost I don't know if this is expected but this is a surprising behavior.
The text was updated successfully, but these errors were encountered: