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

requiredOptionMarker displayed on ArgGroup when it should not #1380

Closed
StaffanArvidsson opened this issue Jun 7, 2021 · 8 comments · Fixed by #1505
Closed

requiredOptionMarker displayed on ArgGroup when it should not #1380

StaffanArvidsson opened this issue Jun 7, 2021 · 8 comments · Fixed by #1505
Labels
theme: usagehelp An issue or change related to the usage help message type: bug 🐛
Milestone

Comments

@StaffanArvidsson
Copy link

When having a ArgGroup with multiplicity 0..1, the usage shows options as required, but they should not. Here's an example code (I'm running picocli version 4.6.1):

@Command(requiredOptionMarker = '*')
public class RequiredMarkerDisplayedIncorrectly {
	
	public static class ExclusiveOptions {

		@Option(names = "--silent", 
				description = "Silent mode",
				required = false
				)
		public boolean silent;

		@Option(names = {"-v","--verbose"}, 
				description = "Verbose mode",
				required = false)
		public boolean verbose;

	}
	
	@ArgGroup(exclusive = true, multiplicity = "0..1")
	public ExclusiveOptions exclusive;
	
	public static void main(String[] args) {
		new CommandLine(new RequiredMarkerDisplayedIncorrectly()).usage(System.out);
	}
}

The output is this:

Usage: <main class> [--silent | -v]
*     --silent    Silent/quiet mode
* -v, --verbose   Verbose mode
@remkop remkop added type: bug 🐛 theme: usagehelp An issue or change related to the usage help message labels Jun 7, 2021
@remkop
Copy link
Owner

remkop commented Jun 7, 2021

Interesting, that certainly looks like a bug.
Would you be interested in providing a pull request for this?

@StaffanArvidsson
Copy link
Author

I could probably fix a pull req for this, but I'm wondering whether there's a corner case that I could have missed here. Or should the option of 'required' or not come completely from some other place than the ArgGroup annotation?

@remkop
Copy link
Owner

remkop commented Jun 7, 2021

I'm wondering whether there's a corner case that I could have missed here. Or should the option of 'required' or not come completely from some other place than the ArgGroup annotation?

Sorry, I don't understand the question. I expected the usage help message to look like this:


Usage: <main class> [--silent | -v]
      --silent    Silent/quiet mode
  -v, --verbose   Verbose mode

So, no * in front of the options, otherwise the same. Is that your expectation also?

@StaffanArvidsson
Copy link
Author

Yes I expect the same behavior in this case! Just pondering if there's some other case where there's some logic needed to deduce if one or more options should show as being required. But I would assume such logic is solved using the @option or @parameter annotation information instead.

@remkop
Copy link
Owner

remkop commented Jun 7, 2021

Yes, the OptionSpec and PositionalParamSpec model classes have an isRequired method (from memory, away from pc).

If you’d be interested in providing a Pr that’d be great!

@StaffanArvidsson
Copy link
Author

So I started digging through the code and found on line 10098 in the ArgGroupSpec that exclusive groups are set to required=true always. When removing this the output instead turns to

Usage: <main class> [[--silent] | [-v]]
      --silent    Silent mode
  -v, --verbose   Verbose mode

Which I guess is correct, as neither of them is actually required so the hard brackets should be included? However, it wasn't that straightforward to set up the project in Eclipse so I cannot run any test suites or verify that this doesn't break anything else (which isn't really included in the contributing info anyways). Do you have any pointers on how to easily set up the project to run tests etc in an efficient way?

@remkop
Copy link
Owner

remkop commented Jun 8, 2021

Hm I have not used Eclipse in a while.
I use Intelli/J. (The Community Edition of that is free.)

You can run gradlew clean build on the command line...

However, there is a reason that we made options required in exclusive groups. I believe here is the relevant ticket: #871
So perhaps the solution needs to be something else, to stop printing the * in this edge case...

@StaffanArvidsson
Copy link
Author

Thanks for giving some hints on how to run the tests, makes it much easier to contribute!

And yeah, never thought of using the groups to handle lists of exclusive options, that's why I asked if there might be some case of which I haven't yet considered and thus might break something when solving my issue. My case seems much more trivial than the #871 ticket, and the validation seems to work in my case. Only the rendering of the usage text is the issue here. Not sure if I can contribute more to this, would require quite a bit of time.

ahmede41 added a commit to ahmede41/picocli that referenced this issue Dec 8, 2021
@remkop remkop linked a pull request Feb 10, 2022 that will close this issue
remkop added a commit to ahmede41/picocli that referenced this issue Feb 10, 2022
remkop added a commit that referenced this issue Feb 10, 2022
[#1380] Fix for requiredOptionMarker displayed on ArgGroup
remkop added a commit that referenced this issue Feb 10, 2022
@remkop remkop added this to the 4.7 milestone Feb 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme: usagehelp An issue or change related to the usage help message type: bug 🐛
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants