Revisit positional arguments

- Add better mapping logic
- Add better type conversion
- More docs for arity and positional option configuration
- Fixes #616

Author: jvalkeal

spring-shell-core/src/main/java/org/springframework/shell/command/CommandParser.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2022 the original author or authors.
+ * Copyright 2022-2023 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -269,7 +269,10 @@ public CommandParserResults parse(List<CommandOption> options, String[] args) {
 					// don't do anything if first arg looks like an option as if we are here
 					// then we'd might remove wrong required option
 					if (!oargs.isEmpty() && !oargs.get(0).startsWith("-")) {
-						results.add(new DefaultCommandParserResult(o, oargs.stream().collect(Collectors.joining(" "))));
+						// as we now have a candicate option, try to see if there is a
+						// conversion we can do and the use it.
+						Object value = convertOptionType(o, oargs);
+						results.add(new DefaultCommandParserResult(o, value));
 						requiredOptions.remove(o);
 					}
 				});
@@ -285,6 +288,15 @@ public CommandParserResults parse(List<CommandOption> options, String[] args) {
 			return new DefaultCommandParserResults(results, positional, errors);
 		}
 
+		private Object convertOptionType(CommandOption option, Object value) {
+			if (conversionService != null && option.getType() != null && value != null) {
+				if (conversionService.canConvert(value.getClass(), option.getType().getRawClass())) {
+					value = conversionService.convert(value, option.getType().getRawClass());
+				}
+			}
+			return value;
+		}
+
 		private static class ParserResult {
 			private CommandOption option;
 			private List<String> args;
@@ -335,12 +347,7 @@ ParserResults visit(List<List<String>> lexerResults, List<CommandOption> options
 								if (holder.error != null) {
 									return Stream.of(ParserResult.of(o, subArgs, null, holder.error));
 								}
-								Object value = holder.value;
-								if (conversionService != null && o.getType() != null && value != null) {
-									if (conversionService.canConvert(value.getClass(), o.getType().getRawClass())) {
-										value = conversionService.convert(value, o.getType().getRawClass());
-									}
-								}
+								Object value = convertOptionType(o, holder.value);
 								Stream<ParserResult> unmapped = holder.unmapped.stream()
 									.map(um -> ParserResult.of(null, Arrays.asList(um), null, null));
 								Stream<ParserResult> res = Stream.of(ParserResult.of(o, subArgs, value, null));

spring-shell-core/src/test/java/org/springframework/shell/command/AbstractCommandTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2022 the original author or authors.
+ * Copyright 2022-2023 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -60,6 +60,8 @@ protected static class Pojo1 {
 		public int method7Arg3;
 		public int method8Count;
 		public float[] method8Arg1;
+		public int method9Count;
+		public String[] method9Arg1;
 
 		public void method1(CommandContext ctx) {
 			method1Ctx = ctx;
@@ -107,5 +109,10 @@ public void method8(float[] arg1) {
 			method8Arg1 = arg1;
 			method8Count++;
 		}
+
+		public void method9(String[] arg1) {
+			method9Arg1 = arg1;
+			method9Count++;
+		}
 	}
 }

spring-shell-core/src/test/java/org/springframework/shell/command/CommandExecutionTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2022 the original author or authors.
+ * Copyright 2022-2023 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -195,7 +195,47 @@ public void testMethodMultiPositionalArgsAll() {
 			.build();
 		execution.evaluate(r1, new String[]{"myarg1value1", "myarg1value2"});
 		assertThat(pojo1.method4Count).isEqualTo(1);
-		assertThat(pojo1.method4Arg1).isEqualTo("myarg1value1 myarg1value2");
+		assertThat(pojo1.method4Arg1).isEqualTo("myarg1value1,myarg1value2");
+	}
+
+	@Test
+	public void testMethodMultiPositionalArgsAllToArray1() {
+		CommandRegistration r1 = CommandRegistration.builder()
+			.command("command1")
+			.description("help")
+			.withOption()
+				.longNames("arg1")
+				.description("some arg1")
+				.position(0)
+				.arity(OptionArity.ONE_OR_MORE)
+				.and()
+			.withTarget()
+				.method(pojo1, "method9")
+				.and()
+			.build();
+		execution.evaluate(r1, new String[]{"myarg1value1", "myarg1value2"});
+		assertThat(pojo1.method9Count).isEqualTo(1);
+		assertThat(pojo1.method9Arg1).isEqualTo(new String[] { "myarg1value1", "myarg1value2" });
+	}
+
+	@Test
+	public void testMethodMultiPositionalArgsAllToArray2() {
+		CommandRegistration r1 = CommandRegistration.builder()
+			.command("command1")
+			.description("help")
+			.withOption()
+				.longNames("arg1")
+				.description("some arg1")
+				.position(0)
+				.arity(OptionArity.ONE_OR_MORE)
+				.and()
+			.withTarget()
+				.method(pojo1, "method8")
+				.and()
+			.build();
+		execution.evaluate(r1, new String[]{"1", "2"});
+		assertThat(pojo1.method8Count).isEqualTo(1);
+		assertThat(pojo1.method8Arg1).isEqualTo(new float[] { 1, 2 });
 	}
 
 	@Test

spring-shell-core/src/test/java/org/springframework/shell/command/CommandParserTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2022 the original author or authors.
+ * Copyright 2022-2023 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -198,7 +198,8 @@ public void testNonMappedArgWithoutOption() {
 		CommandParserResults results = parser.parse(options, args);
 		assertThat(results.results()).hasSize(1);
 		assertThat(results.results().get(0).option()).isSameAs(option1);
-		assertThat(results.results().get(0).value()).isEqualTo("value foo");
+		// no type so we get raw list
+		assertThat(results.results().get(0).value()).isEqualTo(Arrays.asList("value", "foo"));
 		assertThat(results.positional()).containsExactly("value", "foo");
 	}
 
@@ -210,7 +211,7 @@ public void testNonMappedArgWithoutOptionHavingType() {
 		CommandParserResults results = parser.parse(options, args);
 		assertThat(results.results()).hasSize(1);
 		assertThat(results.results().get(0).option()).isSameAs(option1);
-		assertThat(results.results().get(0).value()).isEqualTo("value foo");
+		assertThat(results.results().get(0).value()).isEqualTo("value,foo");
 		assertThat(results.positional()).containsExactly("value", "foo");
 	}
 
@@ -331,10 +332,35 @@ public void testArityErrors() {
 		assertThat(results2.results().get(0).value()).isNull();
 	}
 
+	@Test
+	public void testMapToIntArray() {
+		CommandOption option1 = CommandOption.of(
+				new String[] { "arg1" },
+				null,
+				null,
+				ResolvableType.forType(int[].class),
+				false,
+				null,
+				0,
+				1,
+				2,
+				null,
+				null);
+
+
+		List<CommandOption> options = Arrays.asList(option1);
+		String[] args = new String[]{"1", "2"};
+		CommandParserResults results = parser.parse(options, args);
+		assertThat(results.errors()).hasSize(0);
+		assertThat(results.results()).hasSize(1);
+		assertThat(results.results().get(0).option()).isSameAs(option1);
+		assertThat(results.results().get(0).value()).isEqualTo(new int[] { 1, 2 });
+	}
+
 	@Test
 	public void testMapPositionalArgs1() {
-		CommandOption option1 = longOption("arg1", 0, 1, 1);
-		CommandOption option2 = longOption("arg2", 1, 1, 2);
+		CommandOption option1 = longOption("arg1", ResolvableType.forType(String.class), false, 0, 1, 1);
+		CommandOption option2 = longOption("arg2", ResolvableType.forType(String.class), false, 1, 1, 2);
 		List<CommandOption> options = Arrays.asList(option1, option2);
 		String[] args = new String[]{"--arg1", "1", "2"};
 		CommandParserResults results = parser.parse(options, args);
@@ -346,9 +372,25 @@ public void testMapPositionalArgs1() {
 	}
 
 	@Test
-	public void testMapPositionalArgs2() {
+	public void testMapPositionalArgs11() {
 		CommandOption option1 = longOption("arg1", 0, 1, 1);
 		CommandOption option2 = longOption("arg2", 1, 1, 2);
+		List<CommandOption> options = Arrays.asList(option1, option2);
+		String[] args = new String[]{"--arg1", "1", "2"};
+		CommandParserResults results = parser.parse(options, args);
+		assertThat(results.results()).hasSize(2);
+		assertThat(results.results().get(0).option()).isSameAs(option1);
+		assertThat(results.results().get(1).option()).isSameAs(option2);
+		assertThat(results.results().get(0).value()).isEqualTo("1");
+		// no type so we get raw list
+		assertThat(results.results().get(1).value()).isEqualTo(Arrays.asList("2"));
+	}
+
+	@Test
+	public void testMapPositionalArgs2() {
+		CommandOption option1 = longOption("arg1", ResolvableType.forType(String.class), false, 0, 1, 1);
+		CommandOption option2 = longOption("arg2", ResolvableType.forType(String.class), false, 1, 1, 2);
+
 		List<CommandOption> options = Arrays.asList(option1, option2);
 		String[] args = new String[]{"1", "2"};
 		CommandParserResults results = parser.parse(options, args);

spring-shell-docs/src/main/asciidoc/using-shell-options-arity.adoc

@@ -4,7 +4,7 @@ ifndef::snippets[:snippets: ../../test/java/org/springframework/shell/docs]
 
 Sometimes, you want to have more fine control of how many parameters with an option
 are processed when parsing operations happen. Arity is defined as min and max
-values, where min must be a positive integer and max has to be more or equal to min.
+values, where min must be zero or a positive integer and max has to be more or equal to min.
 
 ====
 [source, java, indent=0]
@@ -14,7 +14,7 @@ include::{snippets}/OptionSnippets.java[tag=option-registration-arityints]
 ====
 
 Arity can also be defined as an `OptionArity` enum, which are shortcuts
-within the following table:
+shown in below table <<using-shell-options-arity-optionarity-table>>.
 
 ====
 [source, java, indent=0]
@@ -23,6 +23,7 @@ include::{snippets}/OptionSnippets.java[tag=option-registration-arityenum]
 ----
 ====
 
+[[using-shell-options-arity-optionarity-table]]
 .OptionArity
 |===
 |Value |min/max
@@ -51,3 +52,35 @@ The annotation model supports defining only the max value of an arity.
 include::{snippets}/OptionSnippets.java[tag=option-with-annotation-arity]
 ----
 ====
+
+One of a use cases to manually define arity is to impose restrictions how
+many parameters option accepts.
+
+====
+[source, java, indent=0]
+----
+include::{snippets}/OptionSnippets.java[tag=option-registration-aritystrings-sample]
+----
+====
+
+In above example we have option _arg1_ and it's defined as type _String[]_. Arity
+defines that it needs at least 1 parameter and not more that 2. As seen in below
+spesific exceptions _TooManyArgumentsOptionException_ and
+_NotEnoughArgumentsOptionException_ are thrown to indicate arity mismatch.
+
+====
+[source, bash]
+----
+shell:>e2e reg arity-errors --arg1
+Not enough arguments --arg1 requires at least 1.
+
+shell:>e2e reg arity-errors --arg1 one
+Hello [one]
+
+shell:>e2e reg arity-errors --arg1 one two
+Hello [one, two]
+
+shell:>e2e reg arity-errors --arg1 one two three
+Too many arguments --arg1 requires at most 2.
+----
+====

spring-shell-docs/src/main/asciidoc/using-shell-options-positional.adoc

@@ -10,3 +10,61 @@ Positional information is mostly related to a command target method:
 include::{snippets}/OptionSnippets.java[tag=option-registration-positional]
 ----
 ====
+
+NOTE: Be careful with positional parameters as it may soon
+become confusing which options those are mapped to.
+
+Usually arguments are mapped to an option when those are defined in a
+command line whether it's a long or short option. Generally speaking
+there are _options_, _option arguments_ and _arguments_ where latter
+are the ones which are not mapped to any spesific option.
+
+Unrecognised arguments can then have a secondary mapping logic where
+positional information is important. With option position you're
+essentially telling command parsing how to interpret plain raw
+ambiguous arguments.
+
+Let's look what happens when we don't define a position.
+
+====
+[source, java, indent=0]
+----
+include::{snippets}/OptionSnippets.java[tag=option-registration-aritystrings-noposition]
+----
+====
+
+Option _arg1_ is required and there is no info what to do with argument
+`one` resulting error for missing option.
+
+====
+[source, bash]
+----
+shell:>arity-strings-1 one
+Missing mandatory option --arg1.
+----
+====
+
+Now let's define a position `0`.
+
+====
+[source, java, indent=0]
+----
+include::{snippets}/OptionSnippets.java[tag=option-registration-aritystrings-position]
+----
+====
+
+Arguments are processed until we get up to 2 arguments.
+
+====
+[source, bash]
+----
+shell:>arity-strings-2 one
+Hello [one]
+
+shell:>arity-strings-2 one two
+Hello [one, two]
+
+shell:>arity-strings-2 one two three
+Hello [one, two]
+----
+====