Add useful error messages when using list of arguments

[StaffanArvidsson]

When having a list of arguments of a certain type, and the type conversion fails at one argument the TypeConversionException is converted to a UnmachedException - leading to a generic error text such as;
Unmatched argument at index 2: 'Bad input'. I made a small example to show the different cases:

public class BadErrorMsg {
	
	// Case 1
	@Option(names="-t",
			split="\\s",
			converter = MyConverter.class
			)
	public List<MyType> args;
	
	// Case 2
//	@Option(names="-t",
//			converter = MyConverter.class
//			)
//	public MyType args;
	
	static class MyType {
		
		String lowCaseStr;
		
		private MyType(String txt){
			lowCaseStr = txt;
		}
		
		public static class MyConverter implements ITypeConverter<MyType>{

			@Override
			public MyType convert(String arg) throws Exception {
				if (!arg.toLowerCase().equals(arg))
					throw new TypeConversionException("Text must be lower case");
				
				return new MyType(arg);
			}
			
		}
		public String toString() {return lowCaseStr;}
	}
	
	public static void main(String[] args) {
		BadErrorMsg msgs = new BadErrorMsg();
		// Case 1 - list
		new CommandLine(msgs).execute("-t","some txt", "Bad input");
//		Results in message: Unmatched argument at index 2: 'Bad input'
		// Case 2 - single argument
//		new CommandLine(msgs).execute("-t","Bad input");
//		Results in message: Invalid value for option '-t' (<args>): Text must be lower case

	}

}

It would be useful to have a way to discern between what is a truly unmatched input (e.g. a miss-spelled argument that is sent to the ITypeConverter by mistake) and simply that the input is poorly formatted. I understand that in the general case this might be hard to tell, but if the ITypeConverter have the logic to decide between the two cases it can give a better/more useful help text. Furthermore, if the 'Bad input' is given as the first argument to -t it will provide the more useful error message and not the Unmatched message.

[remkop]

One way I can think of to improve the error message is to have more information in the error message of the type converter:

if (!arg.toLowerCase().equals(arg))
        throw new TypeConversionException("'" + arg + "' must be lower case");
        // or: throw new TypeConversionException("Text must be lower case but was '" + arg + "'");

Thoughts?

[remkop]

Oh wait, sorry.

... type conversion fails at one argument the TypeConversionException is converted to a UnmachedException - leading to a generic error text

Good catch, perhaps we can append the error message of the TypeConversionException to that generic text, that should result in a more useful message like Unmatched argument at index 2: 'Bad input' (Text must be lower case).

Would you be interested in providing a pull request for this?

[StaffanArvidsson]

Hmm, I would guess that appending the original message could help - in some cases. But there are two separate cases that I see;

1. The input is sent to the wrong ITypeConverter because the input was not of that type, perhaps due to a miss-spell of the next option and that 'flag' is sent as input instead of being recognized as a completely different parameter.
2. The input was sent to the correct converter, but the input was invalid for some reason.

For case (2) the appending of the original message would give better feedback, but there's no tracing back to which flag the argument was sent to. Would be more useful with Invalid value for option '-t' (<args>): Text must be lower case but was 'Bad'.
For case (1) that output could potentially be very misleading, .e.g it was a miss-spelling of a completely different parameter, but you get an error message for a different one.

One way to counter both these possible scenarios would be to throw different exceptions for the two cases. Perhaps sub-classing the TypeConversionException with a specialized exception for case (2) to keep API consistency would be the best solution?

[remkop]

Adding another exception class would be one idea. It would be best if we can avoid that, would need to look in more detail...

Agreed that Unmatched argument at index 2: 'Bad input' does not tell what option caused the error. Something like this would be better: option '-t': unmatched argument at index 2: 'Bad input' (or something like that). Would we need another exception class for this?

Explicitly mentioning the name of the option whose parameter could not be type converted should solve both case 1 and 2, would it not?

[StaffanArvidsson]

I'm not exactly sure of the flow through the Interpreter. If the reason for the UnmachedException is that if there's a TypeConversionException thrown from a List<T> type option, is that argument pushed back on the Stack of arguments and re-evaluated as not part of that option? And ideally, I would like to get that message from the original TypeConversionException that potentially provides useful information of why the thing failed in the first place (but that's only for case (2) - for case (1) that message would likely be misleading).

Regarding the option '-t': unmatched argument at index 2: 'Bad input' output - in case (1) that the "Bad input" was sent to the converter by mistake, I guess the user could guess that 'Bad input' was sent to the wrong option in some way, but it would be asking a bit of the user and ideally I'd like to make it as simple as possible when we could provide better feedback.

The current workaround for this would be to implement and set the parameterConsumer for each List-type option?

[remkop]

is that argument pushed back on the Stack of arguments and re-evaluated as not part of that option?

I remember now; yes, I think that is what is happening.
Picocli also needs to handle cases where an option can take a variable number of option arguments, as well as positional parameters; where the positional parameters have their own type conversion.

One idea is to modify the Interpreter to remember the exception as well as the option for which the argument is pushed back onto the stack; then, if the argument cannot be matched to any other option or positional parameter, show an error message that says something to the effect of "I tried this option, that gave me this error message, then I tried to treat it as a positional parameter, and that gave this other error..."

[MadFoal]

@remkop I would like to try working on this issue.

[MadFoal]

I have noticed that during the trace. The line:

if (!arg.toLowerCase().equals(arg))
                    throw new CommandLine.TypeConversionException("Text must be lower case");

Does not get called for the third parameter, Bad input from new CommandLine(msgs).execute("-t","some txt", "Bad input");

I will continue to look at this issue.

[remkop]

Great, thank you for looking at this, @MadFoal!

[MadFoal]

I am still looking at this issue. So when we use a different class, in the example MyType, commandline does not process the second text argument "Bad Input." If you replace it with a "-t" it will work correctly.

Additionally, if you instead of using MyType, instead use a primitive it works correctly as shown:

// Fixed example use case
public class Issue1383 implements Runnable{

    @CommandLine.Option(names="-t",
            split="\\s"
            ,converter = TestLowerCaseConverter.class
    )

    List<String> args;

    static class TestStringList {
        @CommandLine.Option(names = "-t", split = "\\s", converter = MyType.MyConverter.class, description = "List of Strings")
        ArrayList<MyType> args;
    }

    public static class TestLowerCaseConverter implements CommandLine.ITypeConverter<String> {

        @Override
        public String convert(String arg) throws Exception {
            if (!arg.toLowerCase().equals(arg))
                throw new CommandLine.TypeConversionException("Text must be lower case");

            return new String(arg);
        }

    }

    public void run() {
        int a = 0;
    }


    public static void main(String[] args) {
        Issue1383 msgs = new Issue1383();

        new CommandLine(msgs).execute("-t","some txt", "-t","Bad input");
	/*
	/ Invalid value for option '-t' (<args>): Text must be lower case
	/ Usage: <main class> [-t=<args>[\s<args>...]]...
 	/ -t=<args>[\s<args>...]
	*/ 
	/ 
    }

}

[MadFoal]

@remkop So this is interesting. When you run the test "-t","some txt", "Bad input" this does not actually correctly represent how the compile and code should be working. Ideally, you want to send your arguments with a comma, no space so that when they are passed to the commandline function it is "-t","some txt,Bad input" which would correctly work. The issue has to do with how we handle input and what we use to determine when we should move onto the next distinct argument. Our parser does not simply absorb all the arguments and then try its best to fit to the structure. Instead, once it has all the arguments it will then programmatically go through each one and build the data structure based on the rules from the customer class.

In section 6 of the manual it goes through all the multiple argument permutations in detail, and what we are attempting to do here is not explicitly permitted, using both flags -t and parameters with an arity 1..*

Please feel free to correct me. I can open a PR to work on the documentation and build some additional tests cases to highlight exactly should pass for multiple test cases.

[remkop]

@MadFoal good catch!

What you are saying is that @StaffanArvidsson's conclusion that type conversion failed for the "Bad input" argument is incorrect. The custom type converter never even saw this argument. The default arity for an option of type List is 1, so only one parameter is processed for each occurrence of the -t option. In the original question, that parameter is "some txt".

I am guessing that the split="\\s" is intended to allow the -t option to have an arbitrary number of space-separated arguments. However, that is not how picocli works. Picocli will match one argument for -t, and that argument is then split into two parts because split="\\s" is specified. On the command line you would need to pass "some txt" as a quoted argument to get this behaviour, otherwise picocli would only assign "some" to the -t option. The split="\\s" will not result in picocli consuming multiple arguments for the -t option. If that is what is desired, then you should set arity = "1..*" for that option.

@StaffanArvidsson does this make sense?

[StaffanArvidsson]

So, picocli is very flexible indeed and it's difficult to get a good overview of how to configure things at all levels. Also, java is slightly difficult to debug when comparing the final product and behaviour in the terminal vs what simple unit-tests do (in terms of eating quotes etc).

In this case however, I changed my code slightly to check if your logic actually make sense and let's consider this example;

public class BadErrorMsgTest {
	
	// Case 1
	@Option(names="-t",
			split="\\s",
			converter = MyConverter.class,
			arity="1..*"
			)
	public List<MyType> args;
	
	// Case 2
//	@Option(names="-t",
//			converter = MyConverter.class
//			)
//	public MyType args;
	
	static class MyType {
		
		String lowCaseStr;
		
		private MyType(String txt){
			lowCaseStr = txt;
		}
		
		public static class MyConverter implements ITypeConverter<MyType>{

			@Override
			public MyType convert(String arg) throws Exception {
				System.out.printf("converting argument %s%n",arg);
				if (!arg.toLowerCase().equals(arg))
					throw new TypeConversionException("Text must be lower case but was '" + arg + "'");
				
				return new MyType(arg);
			}
			
		}
		public String toString() {return lowCaseStr;}
	}
	
	public static void main(String[] args) {
		BadErrorMsg msgs = new BadErrorMsg();
		// Case 1 - list
//		new CommandLine(msgs).execute("-t","some txt", "Bad input");
		// Case 2 - single argument
		new CommandLine(msgs).execute("-t", "first-arg", "second-arg","some txt","Bad input");
	}
}

So now using arity="0..*" (tried 1..* as well, same result). The output I get is this;

converting argument first-arg
converting argument second-arg
converting argument second-arg
converting argument some
converting argument txt
converting argument some
converting argument txt
converting argument Bad
Unmatched argument at index 4: 'Bad input'
Usage: <main class> [-t=<args>[\s<args>...]...]...
  -t=<args>[\s<args>...]...

So to me this says that the converter actually sees all arguments until it fails (but with the "smart unquote" leading to "Bad input" being split into two). My guess is that the parser will continue until the exception being thrown and then back up and assume that "Bad" was not supposed to be sent to the -t flag. It's interesting that the output still give back the "Bad input" as a single argument, so unquote is happening on a lower level somehow?

[StaffanArvidsson]

So @remkop I do not think it solves the original problem/feature of providing better feedback to the user, the updated example code shows that there is a possibility for the ITypeConverter to throw a different exception or some other means of going about this. The parser should get that TypeConversionException and it has to decide if simply the "last" argument was not supposed to be part of the -t parameter or if indeed that exception (and message) is the correct one to display to the user (in this case it assumes it to be unmatched).

[MadFoal]

I wrote a PR for this issue. There are two ways to achieve the convert error test message, one way is to use the flag each time you want to supply an argument, and the second way is to use a comma delimiter instead of a white space character.

[remkop]

(...) the updated example code shows that there is a possibility for the ITypeConverter to throw a different exception or some other means of going about this. The parser should get that TypeConversionException and it has to decide if simply the "last" argument was not supposed to be part of the -t parameter or if indeed that exception (and message) is the correct one to display to the user (in this case it assumes it to be unmatched).

@StaffanArvidsson Yes, that is exactly what happens. When arity = "0..*", the -t option can take multiple parameters, so there is some parser ambiguity: for each command line argument, picocli needs to decide whether to assign this argument to the -t option or not. Picocli does this by first checking if the argument is a known option name or subcommand name, and if not, then by "trying" to assign the value by doing the type conversion. If this type conversion succeeds, then the value is assumed to be a parameter of the -t option.

If the type conversion fails, then the parser assumes that it has encountered a command line argument that is not a parameter of the -t option. This argument may be a positional parameter, at this stage the parser does not know yet. All it knows is that the value is not a parameter of the -t option. A message is logged at DEBUG level Bad input cannot be assigned to option -t: type conversion fails: Invalid value for option '-t': Text must be lower case but was 'Bad'.

The parser then proceeds to parse the "Bad input" parameter as a positional parameter, but the command does not have any positional parameters defined, resulting in the "Unmatched argument" error message.

So @remkop I do not think it solves the original problem/feature of providing better feedback to the user (...)

I can see your point of view.

One idea is what @MadFoal suggested: define the option as @Option(names = "-t", split = ",", converter = MyConverter.class) List<MyType> list;. This avoids the parser ambiguity, because we are using the default arity = 1. This way, end users can only specify multiple values by either specifying the option name for each value, like -t val1 -t val2 -t val3 or in a comma-separated list, like -t val1,val2,val3. Any TypeConversionException is now shown to the users, instead of being swallowed and used to disambiguate the input.

Thoughts?

[StaffanArvidsson]

Thank you @remkop for the explanation! I tried to use , for splitting and that does seem to resolve this ambiguity and provide the correct message to the user. However, I cannot change my API at this point so I need to invent my own way of circumvent the logic of the interpreter to work with \\s. Of course this use case was a simplification of the actual code I'm using, I made a slightly more complex test case to better illustrate my use case. But it seems like you have a pretty good idea of it anyways. If you still think it will incur to much complexity for picocli you're free to close the issue.

To provide some context, I'm writing software which performs machine learning tasks using the CLI and some arguments are pretty complex and depend on which packages that have been dynamically loaded. One example is data-transformations that can take sub-arguments, say scaling attributes of certain columns. So I invented a way of specifying these sub-parameters with a syntax like; --transform Scaling:startIndex=1:endIndex=10. It is thus pretty straightforward to see that e.g. the main argument "Scaling" is correct, but perhaps the sub-arguments were invalid somehow. As these sub-arguments are specific per implementation class it is impossible to list them all as separate options so it would be great if I can provide more details and decide what type of bad input was given to return better feedback to the user.

public class ImprovedErrorMsg {

	@Option(names="-t",
			split="\\s", //",", 
			converter = MyConverter.class,
			arity="1..*"
			)
	public List<MyType> args;
	
	@Parameters
    List<String> positional;

	static class MyType {

		String className;
		List<String> subArgs;

		private MyType(String txt){
			className = txt;
		}

		public void setSubArguments(List<String> args) {
			// Do some validation - this would in reality be performed in unique implementation class 
			// Here simply say invalid if given more than 1 sub-argument
			if (args == null)
				return;
			if (args.size()>1) {
				throw new IllegalArgumentException(String.format("Invalid sub-arguments for class %s: %s",className,args));
			}
			// else accept the arguments
			subArgs = args;
		}

		public static class MyConverter implements ITypeConverter<MyType>{

			final static List<String> ALLOWED_VALUES= Arrays.asList("class1","class2");
			final static String SUB_PARAM_SPLITTER = ":";

			@Override
			public MyType convert(String arg) throws Exception {
				// Only there to check if something is checked or not
				// System.out.printf("converting argument %s%n",arg);

				String clsArg = arg;
				List<String> subArgs = null;
				if (arg.contains(SUB_PARAM_SPLITTER)) {
					// Has sub-parameters
					String[] splittedArgs = arg.split(SUB_PARAM_SPLITTER);
					clsArg = splittedArgs[0]; // First portion is the main argument
					subArgs = new ArrayList<>(Arrays.asList(splittedArgs));
					subArgs.remove(0); // remove the main argument
				} 

				for (String cls : ALLOWED_VALUES) {
					if (cls.equalsIgnoreCase(clsArg)) {
						MyType t = new MyType(cls);
						try {
							t.setSubArguments(subArgs);
							return t;
						} catch (IllegalArgumentException e) {
							// Here we _know_ that the main argument was of the correct type,
							// but the sub-argument was invalid somehow. 
							// wish to give more detailed info about these specifics
							throw new TypeConversionException(e.getMessage());
						}
					}
				}
				
				// Not valid - but perhaps we can infer if it was a miss-spelling or non-loaded class or if the text was sent by mistake 
				if (arg.startsWith("class")) {
					throw new TypeConversionException("Input "+arg+" currently not supported");
				}
				
				// Here the thing was likely sent to -t by mistake (or 'tried' and re-evaluated by the interpreter) 
				throw new TypeConversionException("Invalid input: "+arg);
			}

		}
		
		public String toString() {return className;}
	}

	@Test
	public void testCase() {
		ImprovedErrorMsg msgs = new ImprovedErrorMsg();
		// Case 1 - one valid, one which is miss-spelled or not currently available, rest should be directed to the "positional arguments" 
		try {
			new CommandLine(msgs).parseArgs("-t", MyConverter.ALLOWED_VALUES.get(0), "class3", "second-arg","some txt","Bad input");
			Assert.fail("There should be an exception thrown to give more detailed info to the user"); //TODO - this fails as "class3" is directed to "positional"
		} catch (Exception e) {
			e.printStackTrace();
			Assert.assertTrue(e.getMessage().contains("class3 currently not supported"));
		}
		
		// Case 2 - only the concrete classes - no sub-arguments
		msgs = new ImprovedErrorMsg();
		new CommandLine(msgs).parseArgs("-t", MyConverter.ALLOWED_VALUES.get(0), MyConverter.ALLOWED_VALUES.get(1));
		Assert.assertTrue(MyConverter.ALLOWED_VALUES.get(0).equalsIgnoreCase(msgs.args.get(0).className));
		Assert.assertTrue(MyConverter.ALLOWED_VALUES.get(1).equalsIgnoreCase(msgs.args.get(1).className));
		
		// Case 3 - with OK sub-arguments
		msgs = new ImprovedErrorMsg();
		new CommandLine(msgs).parseArgs("-t", MyConverter.ALLOWED_VALUES.get(0)+":param1", MyConverter.ALLOWED_VALUES.get(1)+":param2");
		Assert.assertTrue(MyConverter.ALLOWED_VALUES.get(0).equalsIgnoreCase(msgs.args.get(0).className));
		Assert.assertEquals("param1",msgs.args.get(0).subArgs.get(0));
		Assert.assertTrue(MyConverter.ALLOWED_VALUES.get(1).equalsIgnoreCase(msgs.args.get(1).className));
		Assert.assertEquals("param2",msgs.args.get(1).subArgs.get(0));
		
		// Case 4 - with invalid sub-arguments (i.e. too many in this case)
		msgs = new ImprovedErrorMsg();
		try {
			new CommandLine(msgs).parseArgs("-t", MyConverter.ALLOWED_VALUES.get(0)+":param1:param2", MyConverter.ALLOWED_VALUES.get(1)+":param3");
			Assert.fail("Invalid sub-arguments");
		} catch (Exception e) {
			e.printStackTrace(); // TODO - remove in actual test-case 
			Assert.assertTrue(e.getMessage().contains("Invalid sub-arguments"));
		}
	}

}

This might be outside of the scope of Picocli, that's up to you to decide. But thanks for providing a way of how this can be dealt with!

[remkop]

I see. Another idea is to rearrange the logic a little bit. Specifically, don't specify the type converter in the annotation.
Instead, move the type conversion and validation logic to a setter method.
For example:

List<MyType> args;

@Option(names="-t", split="\\s", arity="1..*")
public void setArgs(List<String> newValues) throws Exception {
    args = convertList(newValues, MyType.class, new MyConverter());
}

static <T> List<T> convertList(List<String> values, Class<T> cls, ITypeConverter<T> converter) throws Exception {
    List<T> result = new ArrayList<>();
    for (String s : values) {
        result.add(converter.convert(s));
    }
    return result;
}

This has a bit more code than the original annotated fields approach, but should give better error messages.

[StaffanArvidsson]

Ok thanks! I looked into trying that instead, and found that making subtypes of the TypeConversionException and using method-annotations did support some level of flexibility as I can then in the error/exception handling check for these subtype exceptions from the ParameterException.getCause() and return that message instead. However, in my example I added these positional arguments as well to simulate more parameters given simultaneously, and using another test-example using my latest test-code;

new CommandLine(msgs).parseArgs("-t", MyConverter.ALLOWED_VALUES.get(0)+":param1", MyConverter.ALLOWED_VALUES.get(1)+":param2", "pos1", "pos2");

will not put the "pos1", "pos2" into the positional @Parameters annotation but instead fail as pos1 could not be converted properly.

[remkop]

will not put the "pos1", "pos2" into the positional @Parameters annotation but instead fail as pos1 could not be converted properly.

What if you don't have split="\\s" for the -t option? (Sorry I don't have bandwidth to try running your code at the moment.)

[StaffanArvidsson]

What if you don't have split="\s" for the -t option?

I think I answered that earlier, when using e.g. split="," the issue is resolved! Though I can't change the API of my software so it doesn't solve it for my use case unfortunately.

[remkop]

Oh I see, yes.
So the task is now finding out why the "pos1", "pos2" arguments are not treated as positional parameters but as parameters for the -t option.

Can you run with -Dpicocli.trace=DEBUG and post the output here?

[StaffanArvidsson]

Here's the debug-log, I also included the stack trace from the exception that is thrown as it might provide additional info. I ran this using the CommandLine.parseArgs(..) method.

[picocli DEBUG] Creating CommandSpec for com.arosbio.modeling.app.ImprovedErrorMsg@3a82f6ef with factory picocli.CommandLine$DefaultFactory
[picocli INFO] Picocli version: 4.6.2, JVM: 1.8.0_121 (Oracle Corporation Java HotSpot(TM) 64-Bit Server VM 25.121-b13), OS: Mac OS X 10.14.6 x86_64
[picocli INFO] Parsing 5 command line args [-t, class1:param1, class2:param2, pos1, pos2]
[picocli DEBUG] Parser configuration: optionsCaseInsensitive=false, subcommandsCaseInsensitive=false, abbreviatedOptionsAllowed=false, abbreviatedSubcommandsAllowed=false, aritySatisfiedByAttachedOptionParam=false, atFileCommentChar=#, caseInsensitiveEnumValuesAllowed=false, collectErrors=false, endOfOptionsDelimiter=--, expandAtFiles=true, limitSplit=false, overwrittenOptionsAllowed=false, posixClusteredShortOptionsAllowed=true, separator=null, splitQuotedStrings=false, stopAtPositional=false, stopAtUnmatched=false, toggleBooleanFlags=false, trimQuotes=false, unmatchedArgumentsAllowed=false, unmatchedOptionsAllowedAsOptionParameters=true, unmatchedOptionsArePositionalParams=false, useSimplifiedAtFiles=false
[picocli DEBUG] (ANSI is disabled by default: systemproperty[picocli.ansi]=null, isatty=false, TERM=null, OSTYPE=null, isWindows=false, JansiConsoleInstalled=false, ANSICON=null, ConEmuANSI=null, NO_COLOR=null, CLICOLOR=null, CLICOLOR_FORCE=null)
[picocli DEBUG] Initializing command 'null' (user object: com.arosbio.modeling.app.ImprovedErrorMsg@3a82f6ef): 1 options, 1 positional parameters, 0 required, 0 groups, 0 subcommands.
[picocli DEBUG] Initial value not available for method void com.arosbio.modeling.app.ImprovedErrorMsg.setArgs(java.util.List<String>) throws Exception
[picocli DEBUG] Set initial value for field java.util.List<String> com.arosbio.modeling.app.ImprovedErrorMsg.positional of type interface java.util.List to null.
[picocli DEBUG] [0] Processing argument '-t'. Remainder=[class1:param1, class2:param2, pos1, pos2]
[picocli DEBUG] '-t' cannot be separated into <option>=<option-parameter>
[picocli DEBUG] Found option named '-t': method void com.arosbio.modeling.app.ImprovedErrorMsg.setArgs(java.util.List<String>) throws Exception, arity=1..*
[picocli DEBUG] 'class1:param1' doesn't resemble an option: 0 matching prefix chars out of 1 option names
[picocli DEBUG] Split with regex '\s' resulted in 1 parts: [class1:param1]
[picocli INFO] Adding [class1:param1] to method void com.arosbio.modeling.app.ImprovedErrorMsg.setArgs(java.util.List<String>) throws Exception for option -t on ImprovedErrorMsg@3a82f6ef
[picocli DEBUG] Split with regex '\s' resulted in 1 parts: [class2:param2]
[picocli DEBUG] 'class2:param2' doesn't resemble an option: 0 matching prefix chars out of 1 option names
[picocli DEBUG] Split with regex '\s' resulted in 1 parts: [class2:param2]
[picocli INFO] Adding [class2:param2] to method void com.arosbio.modeling.app.ImprovedErrorMsg.setArgs(java.util.List<String>) throws Exception for option -t on ImprovedErrorMsg@3a82f6ef
[picocli DEBUG] Split with regex '\s' resulted in 1 parts: [pos1]
[picocli DEBUG] 'pos1' doesn't resemble an option: 0 matching prefix chars out of 1 option names
[picocli DEBUG] Split with regex '\s' resulted in 1 parts: [pos1]
[picocli INFO] Adding [pos1] to method void com.arosbio.modeling.app.ImprovedErrorMsg.setArgs(java.util.List<String>) throws Exception for option -t on ImprovedErrorMsg@3a82f6ef
[picocli DEBUG] Split with regex '\s' resulted in 1 parts: [pos2]
[picocli DEBUG] 'pos2' doesn't resemble an option: 0 matching prefix chars out of 1 option names
[picocli DEBUG] Split with regex '\s' resulted in 1 parts: [pos2]
[picocli INFO] Adding [pos2] to method void com.arosbio.modeling.app.ImprovedErrorMsg.setArgs(java.util.List<String>) throws Exception for option -t on ImprovedErrorMsg@3a82f6ef
[picocli DEBUG] Initializing binding for option '-t' at index 0 (<args>) on ImprovedErrorMsg@3a82f6ef with empty List
picocli.CommandLine$ParameterException: TypeConversionException: Invalid input: pos1 while processing argument at or before arg[4] 'pos2' in [-t, class1:param1, class2:param2, pos1, pos2]: picocli.CommandLine$TypeConversionException: Invalid input: pos1
	at picocli.CommandLine$ParameterException.create(CommandLine.java:17985)
	at picocli.CommandLine$ParameterException.access$19900(CommandLine.java:17909)
	at picocli.CommandLine$Interpreter.parse(CommandLine.java:13131)
	at picocli.CommandLine$Interpreter.parse(CommandLine.java:13091)
	at picocli.CommandLine$Interpreter.parse(CommandLine.java:12992)
	at picocli.CommandLine.parseArgs(CommandLine.java:1478)
	at com.arosbio.modeling.app.ImprovedErrorMsg.testCaseMessage(ImprovedErrorMsg.java:138)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
	at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:63)
	at org.junit.runners.ParentRunner$4.run(ParentRunner.java:331)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:79)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:329)
	at org.junit.runners.ParentRunner.access$100(ParentRunner.java:66)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:293)
	at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:413)
	at org.eclipse.jdt.internal.junit4.runner.JUnit4TestReference.run(JUnit4TestReference.java:86)
	at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:538)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:760)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:460)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:206)
Caused by: picocli.CommandLine$TypeConversionException: Invalid input: pos1
	at com.arosbio.modeling.app.ImprovedErrorMsg$MyType$MyConverter.convert(ImprovedErrorMsg.java:125)
	at com.arosbio.modeling.app.ImprovedErrorMsg$MyType$MyConverter.convert(ImprovedErrorMsg.java:1)
	at com.arosbio.modeling.app.ImprovedErrorMsg.convertList(ImprovedErrorMsg.java:53)
	at com.arosbio.modeling.app.ImprovedErrorMsg.setArgs(ImprovedErrorMsg.java:47)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at picocli.CommandLine$Model$MethodBinding.set(CommandLine.java:11743)
	at picocli.CommandLine$Model$ArgSpec.setValue(CommandLine.java:8939)
	at picocli.CommandLine$Interpreter.applyValuesToCollectionField(CommandLine.java:14042)
	at picocli.CommandLine$Interpreter.applyOption(CommandLine.java:13646)
	at picocli.CommandLine$Interpreter.processStandaloneOption(CommandLine.java:13522)
	at picocli.CommandLine$Interpreter.processArguments(CommandLine.java:13358)
	at picocli.CommandLine$Interpreter.parse(CommandLine.java:13122)
	... 30 more

I haven't used the @Parameters annotation previously so I might have missed something there? I only included it to kind of resemble a real world use case better. It only looks like this right now;

@Parameters
List<String> positional;

[remkop]

oh wait, arity="1..*", so it will continue to consume as many arguments as possible for the -t option...

[StaffanArvidsson]

Yes, e.g. for my example with data transformations - there can be any number of transformations that the user wish to apply, no way to restrict that in advance. That is also an issue that we've come across before, the error messages and bahaviour of the program differs depending on the order in wish the parameters are supplied to the CLI.

[remkop]

Hm, you can tell your end users to use the -- end-of-options delimiter before the positional parameters.

Other than that, how can the parser determine whether a command line argument should be a parameter of the -t option or a positional parameter?

If there is some way to determine this programmatically, then you can use a parameter preprocessor for the -t option that pushes the argument back on the stack and returns false when the argument is not a -t parameter.

[StaffanArvidsson]

Ok, I'll probably go with implementing those parameter processors for some of these cases, thanks for the tip! I think I've already done that for one of my options but might do that for all of them. This issue was opened in June so I've found some workarounds already but they are a bit ad hoc. Again, picocli is very flexible so it's difficult to get a good grasp on where to make these changes in the best way.

[remkop]

Ok, great to hear you have been able to find solutions.

Are you okay with where we got with this conversation?
Do you have any remaining questions or suggestions for improving picocli?

[StaffanArvidsson]

Yeah I think for this issue we found some sort of workaround.

I have a separate issue with regards to ANSI rendering, I've not used ANIS before so it might be some lack of understanding on my part so I'll have to research it a bit more before opening a separate issue for it. But from spending quite a bit time tracking down strange output I've found that calling render on a long String with @|<code> <text>|@ tags where the <text> contains newlines in form of %n the code seems to have a bug and the output is not correct (looks like it copies from the wrong index in a StringBuffer and parts of the text is duplicated and ANSI codes are off). I encountered this in the Jansi library first and tried to use the picocli renderer instead to check if it worked better - but had the exact same issue. Haven't had time to write a proper test case for it yet so once I get some time I'll post a new issue for it (and also to the Jansi library).

[remkop]

Great. Let me close this issue then.
Looking forward to additional feedback.
Enjoy using picocli! :-)