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

@Parameters validation and error messages if arity > 0 is buggy #203

Closed
AshwinJay opened this issue Oct 13, 2017 · 8 comments
Closed

@Parameters validation and error messages if arity > 0 is buggy #203

AshwinJay opened this issue Oct 13, 2017 · 8 comments
Milestone

Comments

@AshwinJay
Copy link

PicoCLI version: 1.0.1

I have a class annotated with @Parameters(arity = "1..*", description = "The input files") and discovered somethings about the validation of arity >= 1:

  1. Fails to validate in some cases
  2. Validates in some other cases but throws different messages based on whether an empty String ("") or zero-length String[] (new String[0]) is supplied to the parse(xx) method
         class Example {
            @Option(names = {"-h", "--help"}, help = true,
                    description = "Displays this help message and quits.")
            private boolean helpRequested;

            @Option(names = {"-o",
                    "--out-dir"}, required = true, usageHelp = true, description = "The output directory")
            private File outputDir;

            @Parameters(arity = "1..*", description = "The input files")
            private File[] inputFiles;
        }

        //Should've failed as inputFiles were not provided
        new CommandLine(new Example()).parse("-o /tmp");

        //Should've failed as inputFiles were not provided
        new CommandLine(new Example()).parse("-o", " /tmp");

        try {
            new CommandLine(new Example()).parse("");
        } catch (MissingParameterException e) {
            //Type 1 error message.
            e.printStackTrace();
        }

        try {
            new CommandLine(new Example()).parse();
        } catch (MissingParameterException e) {
            //Type 2 error message but the parameters are "essentially" the same as type 1.
            e.printStackTrace();
        }

Type 1 exception:

picocli.CommandLine$MissingParameterException: Missing required parameters at positions 0..*: inputFiles
	at picocli.CommandLine$Interpreter.assertNoMissingParameters(CommandLine.java:2159)
	at picocli.CommandLine$Interpreter.applyOption(CommandLine.java:1745)
	at picocli.CommandLine$Interpreter.processPositionalParameters0(CommandLine.java:1645)
	at picocli.CommandLine$Interpreter.processPositionalParameters(CommandLine.java:1601)
	at picocli.CommandLine$Interpreter.processArguments(CommandLine.java:1594)
	at picocli.CommandLine$Interpreter.parse(CommandLine.java:1501)
	at picocli.CommandLine$Interpreter.parse(CommandLine.java:1484)
	at picocli.CommandLine.parse(CommandLine.java:334)
	at Main.main(Main.java:32)

Type 2 exception:

picocli.CommandLine$MissingParameterException: Missing required options [outputDir, inputFiles]
	at picocli.CommandLine$MissingParameterException.create(CommandLine.java:4086)
	at picocli.CommandLine$MissingParameterException.access$1600(CommandLine.java:4071)
	at picocli.CommandLine$Interpreter.parse(CommandLine.java:1511)
	at picocli.CommandLine$Interpreter.parse(CommandLine.java:1484)
	at picocli.CommandLine.parse(CommandLine.java:334)
	at Main.main(Main.java:39)
@remkop remkop added this to the 2.0.0 milestone Oct 13, 2017
@remkop
Copy link
Owner

remkop commented Oct 13, 2017

I'm testing this with current master (2.0.0-SNAPSHOT) and I can reproduce what you are seeing.

However, I see a problem in the test. The -o option should not have usageHelp = true set. That attribute means that picocli stops parsing when it encounters the -o option...

@remkop remkop closed this as completed in c826f63 Oct 13, 2017
@remkop
Copy link
Owner

remkop commented Oct 13, 2017

After removing the usageHelp=true from the -o option, things worked mostly as expected.

Mostly, but not completely.

I agree that the difference between the Type1 and the Type2 exceptions is strange.

The second case seems okay: both the required -o option and the required positional parameter are reported as missing. This is as expected.

In the first case, an empty string parameter is passed. I would expect this to be parsed as a positional parameter (an <inputFiles> value), resulting in a Missing required option '-o=<outputDir>' exception.
Instead what we are getting is a Missing required parameters at positions 0..*: <inputFiles>. I've tracked down and fixed the bug that causes this.

The fix has been pushed to master and I added your example as a new set of test cases (testIssue203() at the bottom).

Thanks for raising this! Please verify that master behaves as expected now.

@AshwinJay
Copy link
Author

AshwinJay commented Oct 15, 2017

@remkop I think the decision to stop parsing and validating if the usageHelp=true is confusing. The reason I think so, is because I want the user to be able to type -h if he/she does not know the parameters. On the other hand, if the non-parameters are specified correctly then I want Pico to be able to validate them. It looks like usageHelp=true disables all that and takes the mutually exclusive approach.

(edited)
I think usageHelp should be treated as a special paramer. If it is false, then Pico continue with parsing and validating the non-usageHelp parameters and options. Where this is helpful is if the sub-command itself has a help option and also has required parameters. I want to be able to print the sub-commands usageHelp, but that is not possible today because the parse method throws the exception and I don't know which sub-command the user wanted to use.

Regards.

@remkop
Copy link
Owner

remkop commented Oct 16, 2017

You raise some good points. Would you mind creating a new ticket?

@remkop
Copy link
Owner

remkop commented Oct 16, 2017

FYI, I just started to work on #115 which aims to provide convenience methods for printing help and executing Runnable commands, even in applications with nested subcommands.

Some work in progress is now in master. See the CommandLine::parseWithHandler(s) methods.
Your input may shape this effort, especially with regards to error handling.

@remkop
Copy link
Owner

remkop commented Oct 16, 2017

I created #207 to Provide ability to find which subcommand threw a ParameterException.

@remkop
Copy link
Owner

remkop commented Oct 16, 2017

See also my comment here showing some idiomatic code for usageHelp. Let me know if this is not what you have in mind.

@AshwinJay
Copy link
Author

Thanks @remkop !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants