Unexpected error message for invalid positional parameters.

[sbernard31]

Hi,

Maybe I missed something but when one of my positional parameters can not be converted/validated, I get an Unmatched argument at index ... instead of a clear error about the syntax of the parameter.

Using Option returns the expected message.

Let's see this example :

@Command(name = "myCommand",
         mixinStandardHelpOptions = true,
         description = "a command with positional parameter.")
public class Example {

    @Parameters(index = "0..*", description = "list of integers.")
    private List<Integer> params;

    @Option(names = "-o", description = "list of integers.")
    private List<Integer> options;

    public static void main(String... args) {
        Example example = new Example();
        int exitCode = new CommandLine(example).execute(args);

        System.exit(exitCode);
    }
}

Using positional parameter :

> myCommand not_an_int
Unmatched argument at index 0: 'not_an_int'
Usage: myCommand [-hV] [-o=<options>]... [<params>...]
a command with positional parameter.
      [<params>...]   list of integers.
  -h, --help          Show this help message and exit.
  -o=<options>        list of integers.
  -V, --version       Print version information and exit.

Using options :

> myCommand -o not_an_int
Invalid value for option '-o' (<options>): 'not_an_int' is not an int
Usage: myCommand [-hV] [-o=<options>]... [<params>...]
a command with positional parameter.
      [<params>...]   list of integers.
  -h, --help          Show this help message and exit.
  -o=<options>        list of integers.
  -V, --version       Print version information and exit.

Does I miss something or is it a kind of bug ?

[sbernard31]

To be clearer for positional parameter, I expected something like :

Invalid value for argument at index 0 : 'not_an_int' is not an int

instead of

Unmatched argument at index 0: 'not_an_int'

[remkop]

@sbernard31 Thank you for the clarification.

Picocli cannot make that statement because picocli allows multiple positional parameters to be defined with overlapping ranges: it is perfectly valid (although unusual) to have an application like this:

@Command
class A {
    @Parameters(index = "0") int x;
    @Parameters(index = "0") double y;
    @Parameters(index = "0") short z;

    @Unmatched
    List<String> unmatched;
}

For this command, the input <cmd> 123 will result in the x, y and z fields to be populated with the int value, double value and the short value 123 (or 123.0).

For the same class, the input <cmd> abc cannot be matched with any of the positional parameters that capture input at index 0. This will result in the "abc" string to be added to the unmatched list. See also https://picocli.info/#_unmatched_input

For options, the situation is a lot less ambiguous, which allows picocli to provide much clearer error messages.

[sbernard31]

I'm a bit confused because it seems to me that this is a very unusual use case.

I would expect that this behavior was not the default one but could be activated only if needed.

FMPOV, in most case, you define one index by parameter and you rather prefer to have a clear error message for each of your parameter than this flexibility.

Maybe, you will not want to change this default behavior for backward compatibility, but is it at least possible to add a kind of way to handle parameter in a more strict way and so get the right error message ? (e.g. with a setStrictParameterMatching or any better name)

[remkop]

Several things come to mind.

First, do you feel like working on this? I am willing to review pull requests to improve this situation. I currently don't have time to work on this myself.

Second, there may be a workaround: you may be able to get a nicer error message with one of the following mechanisms. Can you give these a try?

• custom parameter consumer or parameter preprocessor
• custom validation in setter method

[sbernard31]

I will try the workaround you propose and let you know my finding.

About providing a PR, this is something that I could maybe do (not sure), but I will need assistance at least to define user API.

I currently don't have time to work on this myself.

Is it something provisional or ? It would be too bad if project stop to be maintained or maybe it is in a "finished" state ?

[sbernard31]

First try with custom setter validation.

I'm not sure I use it in a right way but with this code :

@Command(name = "myCommand", mixinStandardHelpOptions = true, description = "a command with positional parameter.")
public class Example2 implements Runnable {

    @Spec
    CommandSpec spec; // injected by picocli

    @Parameters(index = "0..*", description = "list of integers.")
    public void setParameters(List<String> value) {
        if (!value.isEmpty()) {
            String lastParam = value.get(value.size() - 1);
            try {
                params.add(Integer.parseInt(lastParam));
            } catch (NumberFormatException e) {
                throw new ParameterException(spec.commandLine(), String.format(
                        "Invalid value for parameter at index %s: '%s' is not a number.", value.size() - 1, lastParam));
            }
        }
    }

    private List<Integer> params = new ArrayList<>();

    @Option(names = "-o", description = "list of integers.")
    private List<Integer> options;

    public void run() {
    }

    public static void main(String... args) {
        Example2 example = new Example2();
        int exitCode = new CommandLine(example).execute(args);

        System.exit(exitCode);
    }
}

I get something like :

> myCommand  256 not_an_int
Invalid value for parameter at index 1: 'not_a_int_too' is not a number.
Usage: myCommand [-hV] [-o=<options>]... [<params>...]
a command with positional parameter.
      [<params>...]   list of integers.
  -h, --help          Show this help message and exit.
  -o=<options>        list of integers.
  -V, --version       Print version information and exit.

So this is a possible workaround. (but without converter and I really like converter :grin:)

[remkop]

So this is a possible workaround.

Glad to hear that!

[sbernard31]

Second try with a parameter consumer.

I try to create a generic parameter consumer which could be used to get the desired behavior explained above.

I failed to do because this means rewrite too many code which already exist in picocli and which can not be reused directly.

So I limit myself to a consumer which support only List with one converter :disappointed: ...

The code looks like :

@Command(name = "myCommand", mixinStandardHelpOptions = true, description = "a command with positional parameter.")
public class Example3 implements Runnable {

    public static class ExitOnErrorParameterConsumer implements IParameterConsumer {

        @SuppressWarnings("unchecked")
        public void consumeParameters(Stack<String> args, ArgSpec argSpec, CommandSpec commandSpec) {

            PositionalParamSpec spec = (PositionalParamSpec) argSpec;

            // Get converter;
            ITypeConverter<?>[] converters = argSpec.converters();
            ITypeConverter<?> converter = null;
            if (converters.length > 0) {
                converter = converters[0];
            }

            // Populate parameters value;
            @SuppressWarnings("rawtypes")
            List list = argSpec.getValue();
            for (int i = spec.index().min(); i <= spec.index().max() && !args.isEmpty(); i++) {
                String arg = args.pop();
                try {
                    list.add(converter == null ? arg : converter.convert(arg));
                } catch (Exception e) {
                    String typeDescr = argSpec.auxiliaryTypes()[0].getSimpleName();
                    String msg = String.format(
                            "Invalid value for parameter at index %d: cannot convert '%s' to %s (%s)", i, arg,
                            typeDescr, e);
                    throw new ParameterException(commandSpec.commandLine(), msg, e, argSpec, arg);
                }
            }
        }
    }

    @Parameters(index = "0", description = "first param")
    private Integer param0;

    @Parameters(index = "1..3", description = "list of integers.", parameterConsumer = ExitOnErrorParameterConsumer.class, converter = IntegerConverter.class)
    private List<Integer> params = new ArrayList<>();

    private static class IntegerConverter implements ITypeConverter<Integer> {
        public Integer convert(String value) {
            try {
                return Integer.valueOf(value);
            } catch (Exception ex) {
                throw new TypeConversionException(String.format("%s is not a Integer", value));
            }
        }
    }

    @Parameters(index = "4..*", description = "last param")
    private List<String> strParams;

    @Option(names = "-o", description = "list of integers.")
    private List<Integer> options;

    public void run() {
        System.out.println(param0);
        System.out.println(params);
        System.out.println(strParams);
    }

    public static void main(String... args) {
        Example3 example = new Example3();
        int exitCode = new CommandLine(example).execute(args);

        System.exit(exitCode);
    }
}

With it I get :

> myCommand 111 222 not_a_int 444
Invalid value for parameter at index 2: cannot convert 'not_a_int' to Integer (picocli.CommandLine$TypeConversionException: not_a_int is not a Integer)
Usage: myCommand [-hV] [-o=<options>]... <param0> [<params> [<params>
                 [<params>]]] [<strParams>...]
a command with positional parameter.
      <param0>           first param
      [<params> [<params> [<params>]]]
                         list of integers.
      [<strParams>...]   last param
  -h, --help             Show this help message and exit.
  -o=<options>           list of integers.
  -V, --version          Print version information and exit.

So this is another possible workaround which I feel still not so good as not to generic enough but which could be used with converter.

[sbernard31]

I will not try with a parameter preprocessor because in this case I feel I will not be able to do more than with a parameter consumer.

[remkop]

Interesting! The setter solution looks simpler.

If there are multiple setters there may be common code that can be factored out.

[sbernard31]

As I said I will maybe give a try about writing a PR.

But first, are you still open about this kind of idea ? or maybe now you know there is a workaround you prefer to not change the code ?

To build picocli gradle is needed correct ? I see a pom.xml file but it seems that the project doesn't really work with maven. (I didn't find any developer documentation except the CONTRIBUTING.md)

If gradle is needed, do you know if I should I use a recent version or do you think it should be OK with the one available in debian stable repository ? (strangely I do a lot of java but never used gradle :sweat_smile:)

[remkop]

Well, these days I prefer to spend my time on other things: the pandemic has changed and clarified my priorities in life quite a bit. 😅 More family time and enjoying life, less open source work.

That doesn’t rule out improvements to picocli, it’s just that other things take priority. So things may move slowly.

About this ticket, it looks like (from memory, I haven’t confirmed this) currently picocli considers an argument “unmatched” either by index or by type mismatch.

“By index” means that more parameters were specified on the command line than are defined in the command. For example, the command has positional parameters for indexes 0..2, but the end user specified <cmd> a b c d e: too many.

“By type mismatch” is your use case. To improve this I think it makes sense to verify that there’s only one PositionalParamSpec that covers this index/position, and if so, generate a nicer error message when type conversion fails.

[sbernard31]

Well, these days I prefer to spend my time on other things: the pandemic has changed and clarified my priorities in life quite a bit. sweat_smile More family time and enjoying life, less open source work.

This is totally understandable. :+1:

That doesn’t rule out improvements to picocli, it’s just that other things take priority. So things may move slowly.

Moving slow is still moving. That's fine to me :-)

I will let you know if I move a little bit on this on my side