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

Mixing Required + Non-Required Options in an ArgGroup -- required options not validated #870

Closed
scottjasso opened this issue Nov 19, 2019 · 9 comments
Labels
theme: arg-group An issue or change related to argument groups type: bug 🐛
Milestone

Comments

@scottjasso
Copy link

scottjasso commented Nov 19, 2019

I have a use case similar to other issues -- options with sub-options. For example:
cli --group=alpha --opt1=1 --group=bravo --opt2=foo

I'm trying something like this:

class Group {
    @Option(names = {"--group"}, required = true) String name;
    @Option(names = {"--opt1"}) int opt1;
    @Option(names = {"--opt2"}) String opt2;
}

@ArgGroup(exclusive = false, multiplicity = "0..*") List<Group> groups;

This almost works:

  • cli --group=alpha --opt1=1 --group=bravo --opt2=foo
    results in
    groups = [{name=alpha, opt1=1}, {name=bravo, opt2=foo}]
  • cli --opt1=1 --group=alpha --opt2=foo (weird that --opt1 comes before --group, but allowed)
    results in
    groups = [{name=alpha, opt1=1, opt2=foo}]

However, calling cli --opt1=1 results in [{name=null, opt1=2}], with no error. I'd expect an error since the group name is required but not provided, but that doesn't seem to be the case. Is this supported?

@remkop
Copy link
Owner

remkop commented Nov 19, 2019

Thank you for raising this. That does look odd. I will take a look when I get to my PC.

@remkop remkop added this to the 4.1 milestone Nov 19, 2019
@scottjasso
Copy link
Author

Sorry, should have mentioned I'm using 4.0.4.

@scottjasso
Copy link
Author

Also, to give some context, I'm trying to build a Java equivalent of a CLI previously implemented in python via Click. It uses command chaining and the command group callback feature to accomplish repeated subcommands. AFAICT, repeated subcommands aren't possible in picocli, but this seems like the closest approach.

@remkop
Copy link
Owner

remkop commented Nov 19, 2019

@wjohnson5 Thanks for that background. A feature like this ("repeatable subcommands" #454 ) is on the TODO list but has been shelved until a clear use case comes up. On the surface it does not look that hard to implement but I haven't spent the time to analyse in depth. The Click documentation is certainly helpful.

@remkop remkop added theme: arg-group An issue or change related to argument groups type: bug 🐛 labels Nov 19, 2019
@remkop
Copy link
Owner

remkop commented Nov 19, 2019

I have been able to reproduce the issue and found the cause: a bug in the validation logic.

remkop added a commit that referenced this issue Nov 19, 2019
@remkop
Copy link
Owner

remkop commented Nov 19, 2019

Fixed in master. Can you confirm?

git clone https://github.com/remkop/picocli.git
cd picocli
gradlew clean build publishToMavenLocal

Then test your project with picocli-4.0.5-SNAPSHOT. You should get an error for input like cli --opt1=1.

@remkop remkop closed this as completed Nov 19, 2019
@scottjasso
Copy link
Author

scottjasso commented Nov 19, 2019

Confirmed fixed!
Some other comments:

  • Using exclusive = True with both optional and required parameters probably doesn't make sense as it is right now -- if you change the example to have exclusive = True, calling cli --opt1=1 works and gives [{name=null, opt1=1}], and cli --group=foo --opt1=1 gives [{name=null, opt1=1},{name=foo}]. I'd think, either (1) only the required arguments are exclusive, and it's still an error to specify an ArgGroup option without exactly one of the required args, or maybe (2) picocli should error out if using non-required options w/ exclusive = True.

  • Any thoughts on changing some error messages?

    • If num groups > max(multiplicity) (whether the args are optional or required), it's currently:

      Error: expected only one match but got (--group=<name> [--opt1=<opt>])={--group=foo --opt1=1} and (--group=<name> [--opt1=<opt>])={--opt1=1}

      I like that this shows you the matched groups, but (1) it always says "expected only one match", even if the minimum multiplicity > 1, and (2) it seems unnecessary to repeat the group spec.

      If num groups < min(multiplicity), it's currently:

      Error: Group: (--group=<name> [--opt1=<opt>]) must be specified 2 times but was matched 1 times

      For consistency, I think both cases should be something like:

      Error: (--group=<name> [--opt1=<opt>]) must be specified [X / at least X / at most X] time(s) but was matched 2 times: {--group=foo --opt1=1}, {--opt1=2}

    • If missing a required option for a group, it's currently:

      Error: Missing required argument(s): --group=<name>

      IMO something like

      Error: --group=<name> is required in (--group=<name> [--opt1=<opt>]), got: {--opt1=1}

      This clarifies that --group=<name> is only needed when specifying --opt1. This also applies for mutually dependent args:

      Error: --req-arg1=<foo>, --req-arg2=<bar> are all required in (--req-arg1=<foo>, --req-arg2=<bar> [--opt1=<opt>]), got: {--req-arg1=foo}

  • I noticed a bug with exclusive args and multiplicity (I can open this as a separate issue). For exclusive=true, multiplicity="2", with a group containing (exclusive) required options --req1 and --req2, passing cli --req1 --req2 fails: must be specified 2 times but was matched 1 times

Thanks for the awesome support! I really appreciate the quick responses and turn around time. (I even searched for an option to donate some amount in appreciation -- if you're open to the idea of donations!)

@remkop
Copy link
Owner

remkop commented Nov 19, 2019

Thanks for confirming the fix and thanks for the kind words!

Thanks also for letting me know about the other issues. Would it be possible to raise separate tickets for them? That makes it easier for me to track.

@scottjasso
Copy link
Author

Will do. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme: arg-group An issue or change related to argument groups type: bug 🐛
Projects
None yet
Development

No branches or pull requests

2 participants