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

Grouping arguments with arg groups and getting unexpected behaviour #1815

Closed
rolfyone opened this issue Sep 13, 2022 · 9 comments
Closed

Grouping arguments with arg groups and getting unexpected behaviour #1815

rolfyone opened this issue Sep 13, 2022 · 9 comments
Labels
theme: arg-group An issue or change related to argument groups theme: parser An issue or change related to the parser theme: usagehelp An issue or change related to the usage help message type: doc 📘 type: enhancement ✨
Milestone

Comments

@rolfyone
Copy link

In teku, we're using arg groups to basically categorise command line args into sections to help produce better help information etc...

We have a bunch of overrides etc that are outside of one issue we're having, basically someone has specified the same flag twice, and then, seemingly randomly, an argument is not read from the cli. Consensys/teku#6146

To replicate what we're seeing, I've created a simple project:

// CmdTest.java
import picocli.CommandLine;
import picocli.CommandLine.Command;

import java.util.concurrent.Callable;
@Command(name = "groups", mixinStandardHelpOptions = true,
    description = "groups fun")
public class CmdTest implements Callable<Integer> {

  @CommandLine.ArgGroup(validate = false, heading = "GROUP A%n")
  private GroupA a = new GroupA();

  @CommandLine.ArgGroup(validate = false, heading = "GROUP B%n")
  private GroupB b = new GroupB();

  @Override
  public Integer call() {
    System.out.println(a);
    System.out.println(b);
    return 0;
  }

  public static void main(String[] args) {
    int exitCode = new CommandLine(new CmdTest()).execute(args);
    System.exit(exitCode);
  }
}
// GroupA.java
import picocli.CommandLine;

public class GroupA {
  @CommandLine.Option(
      names = {"--a-str"},
      paramLabel = "<NETWORK>",
      arity = "1")
  private String str = null;

  @CommandLine.Option(
      names = {"--a-bool"},
      paramLabel = "<BOOLEAN>",
      arity = "1",
      fallbackValue = "true")
  private boolean bool = false;

  public GroupA() {
  }

  @Override
  public String toString() {
    final StringBuilder sb = new StringBuilder("GroupA{");
    sb.append("str='").append(str).append('\'');
    sb.append(", bool=").append(bool);
    sb.append('}');
    return sb.toString();
  }
}
// GroupB.java
import picocli.CommandLine;

public class GroupB {
  @CommandLine.Option(
      names = {"--b-bool"},
      paramLabel = "<BOOLEAN>",
      showDefaultValue = CommandLine.Help.Visibility.ALWAYS,
      fallbackValue = "true",
      arity = "1")
  private boolean b = false;

  public GroupB() {
  }

  @Override
  public String toString() {
    final StringBuilder sb = new StringBuilder("GroupB{");
    sb.append("b=").append(b);
    sb.append('}');
    return sb.toString();
  }
}

User runs cmdTest --a-str=asdf --b-bool=true --a-bool=true and everything gets set as expected.

If the user runs cmdTest --a-str=asdf --b-bool=true --a-bool=true --a-bool=true, then the string in the first group is no longer set.

Removing validate=false, and replacing with exclusive=false, we get a complicated error about the group interactions and what is valid / not valid.

My guess is that the interaction of the string ending up being null is to do with something that is a side effect of validate=false.

Printing out help looks like:

Usage: groups [-hV] [--a-bool=<BOOLEAN>] [--a-str=<NETWORK>]
              [--b-bool=<BOOLEAN>]
groups fun
  -h, --help               Show this help message and exit.
  -V, --version            Print version information and exit.
GROUP A
      --a-bool=<BOOLEAN>
      --a-str=<NETWORK>
GROUP B
      --b-bool=<BOOLEAN>     Default: false

which is good, we get groupings etc, and all of the help information is well formatted.

It's definitely possible that we're doing something that the library is really not intended for... It's arguably a 'user error' specifying the same argument twice on the CLI, I'm just not exactly sure how I should approach resolving this kind of issue...

@remkop
Copy link
Owner

remkop commented Sep 13, 2022

Interesting, yes, I see the problem.
When validate = true, you get this error:

Error: expected only one match but got [[--a-str=<NETWORK>] [--a-bool=<BOOLEAN>]]={--a-str=asdf --a-bool=true} [[--b-bool=<BOOLEAN>]]={--b-bool=true} and [[--a-str=<NETWORK>] [--a-bool=<BOOLEAN>]]={--a-bool=true}

This error is trying to say that only one combination of --a-str and --a-bool was expected, but that multiple groups were found (the last one partially matched). And I see that this error message is hard to understand.

Let's see, what can we do?

One idea (which is a bit of a hack) is to capture a List of GroupA instance in CmdTest, as follows:

@ArgGroup(validate = false, heading = "GROUP A%n", multiplicity = "0..1")
private List<GroupA> a = new ArrayList<GroupA>();

Now, for the following input:

--a-str=asdf --b-bool=true --a-bool=true --a-bool=true

... I get this output:

[GroupA{str='asdf', bool=true}, GroupA{str='null', bool=false}, GroupA{str='null', bool=true}]
GroupB{b=true}

Then, in your application logic, you can do custom validation and throw a ParameterException, which will show a nicer error message.

@remkop
Copy link
Owner

remkop commented Sep 13, 2022

But I agree that this is not very nice.

The issue is really, that groups are not just cosmetic, even if your intention is to only use groups for the Option Sections in the usage help... They do interfere with the normal validation, so if options are specified multiple times, the single option validation does not work. (And group validation, if enabled with validate=true, gives that lengthy error message...)

I need to think about this more...

remkop added a commit that referenced this issue Sep 13, 2022
@rolfyone
Copy link
Author

Ok :) I did get kind of lost with the long error message, but got the rough gist, just not sure that I'd show that to a user ideally...

One option was also that we think about going in a different way to get our usage categories and avoiding using the groups altogether... I'll have a play tomorrow with what you're suggesting and also look forward to your further thoughts :)

@rolfyone
Copy link
Author

I have to dig a bit, but potentially something like a label on a mixin would let us accomplish documentation wtihout actually having the arg group logic...
I'm not sure if this exists yet, just throwing it out there to update thought process... i'm planning on digging around and taking a look probably tomorrow if things aren't too crazy...

@remkop
Copy link
Owner

remkop commented Sep 14, 2022

Away from my PC now, but the picocli-examples directory in the project has a customhelp subdirectory that may be useful.
To get custom sections without ArgGroups you may need to override the way the option list is rendered.

@remkop remkop added theme: arg-group An issue or change related to argument groups theme: parser An issue or change related to the parser theme: usagehelp An issue or change related to the usage help message labels Oct 2, 2022
@ajsutton
Copy link

ajsutton commented Dec 6, 2022

Using a custom renderer for the options section worked perfectly and was much simpler than I expected: Consensys/teku#6556

I'd say this issue could either be closed or turned into a feature request to have built in support for a heading option on @Mixin. @ArgGroup is definitely not a good approach.

@remkop
Copy link
Owner

remkop commented Dec 6, 2022

@ajsutton thanks for the feedback! 😅

@remkop
Copy link
Owner

remkop commented Dec 23, 2022

I will update the documentation to make a note that setting validate = false means that picocli won't validate, even in cases when users specify a group with multiplicity = 1 multiple times.

At this time the best workaround is making the field holding the group a collection, and doing custom validation to ensure that this collection holds only a single element.

@ArgGroup(validate = false, heading = "GROUP A%n", multiplicity = "0..1")
private List<GroupA> a = new ArrayList<GroupA>();

@remkop remkop closed this as completed in d70cc29 Dec 24, 2022
@remkop
Copy link
Owner

remkop commented Dec 27, 2022

I added this section to the user manual: https://picocli.info/#_validation_trade_offs

Thank you again for raising this!

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 theme: parser An issue or change related to the parser theme: usagehelp An issue or change related to the usage help message type: doc 📘 type: enhancement ✨
Projects
None yet
Development

No branches or pull requests

3 participants