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

option is treated as option-parameter (was: option is treated as option-parameter in Arguments Groups) #1055

Closed
waacc-gh opened this issue May 18, 2020 · 9 comments
Labels
theme: arg-group An issue or change related to argument groups theme: parser An issue or change related to the parser type: bug 🐛
Milestone

Comments

@waacc-gh
Copy link

see: #1054

    @ArgGroup(exclusive = false, multiplicity = "1..*")
    private List<Modification> modifications = null;

    private static class Modification {
        @Option(names = { "-f", "--find" }, required = true)
        public  Pattern findPattern = null;
        @ArgGroup(exclusive = true, multiplicity = "1")
        private Change  change      = null;
    }

    private static class Change {
        @Option(names = { "-d", "--delete" }, required = true)
        public boolean delete      = false;
        @Option(names = { "-w", "--replace-with" }, required = true)
        public String  replacement = null;
    }

called with:

  • -f pattern -w text --> accepted --> ok
  • -f -f -w text --> accepted --> wrong: findPattern = "-f", means, the second -f is treated as an option-parameter for the first -f
  • -f pattern -w -d --> wrong: replacement = "-d", means -d is treated as an option-parameter for -w
@remkop
Copy link
Owner

remkop commented May 18, 2020

@waacc-gh Thank you for raising this!
I am looking into this...

@remkop remkop added the theme: arg-group An issue or change related to argument groups label May 18, 2020
@remkop
Copy link
Owner

remkop commented May 19, 2020

This behaviour is not limited to Argument Groups but can actually happen with any option that accepts a parameter. For example:

@Command
public class QuickTest {
    @Option(names = "-a") String a;
    @Option(names = "-b") String b;

    public static void main(String[] args) {
        QuickTest quickTest = new QuickTest();

        // value matches existing option
        new CommandLine(quickTest).parseArgs("-a", "-b");
        System.out.printf("-a=%s, -b=%s%n", quickTest.a, quickTest.b);

        // value "resembles" an option
        new CommandLine(quickTest).parseArgs("-a", "-c");
        System.out.printf("-a=%s, -b=%s%n", quickTest.a, quickTest.b);
    }
}

This prints:

-a=-b, -b=null
-a=-c, -b=null

I will add this to the roadmap for the next release.

There is an outstanding request to improve a similar situation where values "resembling options" are accepted as option parameters or as positional parameters.

(Edit) I will probably introduce two separate parser switches to allow/disallow:

  • values that exactly match an existing option
  • values that only "resemble an option"

This will likely need to be enabled explicitly for backwards compatibility (the default behaviour will likely remain the current behaviour).

Thoughts?

@remkop remkop added this to the 4.4 milestone May 19, 2020
@remkop remkop added the theme: parser An issue or change related to the parser label May 19, 2020
@waacc-gh
Copy link
Author

waacc-gh commented May 22, 2020

The reason why I raised this issue is, I'm confused. The following examples behave different. Is this intended? (Yes I know, my example above is a little bit different. But I think it's more or less the same problem.)

@Command
public class Test {
    @Option(names = "-a", required = false)
    public String a = null;
    @Option(names = "-b", required = false)
    String b;

    public static void main(String[] args) {
        Test test = new Test();
        new CommandLine(test).parseArgs("-a", "-b");
        System.out.printf("-a=%s, -b=%s%n", test.a, test.b);
    }
}
@Command
public class Test {
    @Option(names = "-a", required = false)
    public List<String> a = null;

    public static void main(String[] args) {
        Test test = new Test();
        new CommandLine(test).parseArgs("-a", "-a");
        System.out.printf("-a=%s", test.a);
    }
}

@waacc-gh
Copy link
Author

Yes, you are right, the main problem of this issue is, to guess the intention of the user: is a command line argument starting with - (or --) always an option or could it be also a parameter.

I'm not sure if additional parser switch(s) really solve this issue.
So, my ideas/thoughts:

Always treating an argument starting with - as an option is not an good idea, because maybe a parameter really needs to start with -.

A solution might be, to enforce using an (e.g.) = if a parameter needs a leading - (e.g.: -a=-b).
But this doesn't solve the problem for positional parameters.

Next idea: If an option is defined having an option-parameter, then the argument behind the option is always treated as its option-parameter.
But then we might run in trouble with arguments (starting with -) intended as positional parameters.

Putting ambiguous parameters between " might be a solution (e.g.: -a "-b", means -a=-b).
But as far as I know, the " is not handed to java by the command line interpreter.
Maybe using another character instead of "?

Maybe escaping the - (e.g.: -a \-b, means again -a=-b)?

Sorry, till now, I do not have a simple and catchy solution. :-(

@remkop
Copy link
Owner

remkop commented May 22, 2020

I believe the best way forward it to provide parser options to disallow values that are actual options or are similar to options. When applications need to allow such values they can configure the picocli parser accordingly.

I don't think that a general solution exists for this. Any further validation is the responsibility of the application.

@waacc-gh
Copy link
Author

I also do not think that there is a general solution for every use case. And I've learned, it doesn't matter how many use case are considered, there is always another on. But in my opinion, the parse should act the same way in similar case and not different (like in the two example above).

Escaping the - is my best idea till now. I don't know, how is unix/linux dealing with this problem?

@remkop
Copy link
Owner

remkop commented Jun 6, 2020

You raised a good point that picocli's behaviour is currently different for single-value options and multi-value options: -a -a is accepted (and the -a option is given value "-a") if the -a option has type String, but the same input is rejected with error "Expected parameter for option '-a' but found '-a'" when the option has type List<String> or String[].

It is better if picocli would be consistent, so I am thinking now to by default also reject the first case, so that single-value options will also reject values that exactly match an existing option.

That leaves two problems:

  • what if an end user legitimately needs to specify a value that exactly matches an existing option? I did some research but could not find any prior art that even mentions this issue (let me know if you find any). I like your idea that end users would need to escape the hyphen; applications would then need to strip off that escape character (the backslash in your example).
  • picocli currently accepts values that only "resemble an option" as option parameters for both single-value and multi-value options. Some applications want to reject such input. I will change positional param handling (see Varargs positional arguments consume unmatched options #1015) to stop accepting values that "resemble an option" as positional parameters unless unmatchedOptionsArePositionalParams=true. Perhaps we need another parser option for this.

Thoughts?

remkop added a commit that referenced this issue Jun 17, 2020
@remkop remkop changed the title option is treated as option-parameter in Arguments Groups option is treated as option-parameter (was: option is treated as option-parameter in Arguments Groups) Jun 18, 2020
@remkop
Copy link
Owner

remkop commented Jun 18, 2020

I started to work on this.
I pushed a fix to master, but I still need to add tests to verify that user will be able to specify other options as option parameters when the value is quoted. Also potentially some documentation work.

@remkop
Copy link
Owner

remkop commented Jun 19, 2020

Fixed in master. This will be included in the upcoming 4.4 release.

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 type: bug 🐛
Projects
None yet
Development

No branches or pull requests

2 participants