From 95e2829dc3d76de56ee6be4af97aaddca9e1856f Mon Sep 17 00:00:00 2001 From: Janne Valkealahti Date: Fri, 13 Jan 2023 15:11:38 +0000 Subject: [PATCH] Revisit positional arguments - Add better mapping logic - Add better type conversion - More docs for arity and positional option configuration - Fixes #616 --- .../shell/command/CommandParser.java | 23 +++++--- .../shell/command/AbstractCommandTests.java | 9 ++- .../shell/command/CommandExecutionTests.java | 44 +++++++++++++- .../shell/command/CommandParserTests.java | 54 +++++++++++++++-- .../asciidoc/using-shell-options-arity.adoc | 37 +++++++++++- .../using-shell-options-positional.adoc | 58 ++++++++++++++++++ .../shell/docs/OptionSnippets.java | 59 ++++++++++++++++++- .../shell/samples/e2e/ArityCommands.java | 2 + 8 files changed, 266 insertions(+), 20 deletions(-) diff --git a/spring-shell-core/src/main/java/org/springframework/shell/command/CommandParser.java b/spring-shell-core/src/main/java/org/springframework/shell/command/CommandParser.java index 7b97d6c8..2a6c5b61 100644 --- a/spring-shell-core/src/main/java/org/springframework/shell/command/CommandParser.java +++ b/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 interface CommandParser { // 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 interface CommandParser { 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 args; @@ -335,12 +347,7 @@ public interface CommandParser { 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 unmapped = holder.unmapped.stream() .map(um -> ParserResult.of(null, Arrays.asList(um), null, null)); Stream res = Stream.of(ParserResult.of(o, subArgs, value, null)); diff --git a/spring-shell-core/src/test/java/org/springframework/shell/command/AbstractCommandTests.java b/spring-shell-core/src/test/java/org/springframework/shell/command/AbstractCommandTests.java index 2f98939f..c87f3c80 100644 --- a/spring-shell-core/src/test/java/org/springframework/shell/command/AbstractCommandTests.java +++ b/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 @@ public abstract class AbstractCommandTests { 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 abstract class AbstractCommandTests { method8Arg1 = arg1; method8Count++; } + + public void method9(String[] arg1) { + method9Arg1 = arg1; + method9Count++; + } } } diff --git a/spring-shell-core/src/test/java/org/springframework/shell/command/CommandExecutionTests.java b/spring-shell-core/src/test/java/org/springframework/shell/command/CommandExecutionTests.java index d801bac0..3951ac13 100644 --- a/spring-shell-core/src/test/java/org/springframework/shell/command/CommandExecutionTests.java +++ b/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 class CommandExecutionTests extends AbstractCommandTests { .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 diff --git a/spring-shell-core/src/test/java/org/springframework/shell/command/CommandParserTests.java b/spring-shell-core/src/test/java/org/springframework/shell/command/CommandParserTests.java index 832b64ba..8b80b0dd 100644 --- a/spring-shell-core/src/test/java/org/springframework/shell/command/CommandParserTests.java +++ b/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 class CommandParserTests extends AbstractCommandTests { 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 class CommandParserTests extends AbstractCommandTests { 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 class CommandParserTests extends AbstractCommandTests { 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 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 options = Arrays.asList(option1, option2); String[] args = new String[]{"--arg1", "1", "2"}; CommandParserResults results = parser.parse(options, args); @@ -346,9 +372,25 @@ public class CommandParserTests extends AbstractCommandTests { } @Test - public void testMapPositionalArgs2() { + public void testMapPositionalArgs11() { CommandOption option1 = longOption("arg1", 0, 1, 1); CommandOption option2 = longOption("arg2", 1, 1, 2); + List 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 options = Arrays.asList(option1, option2); String[] args = new String[]{"1", "2"}; CommandParserResults results = parser.parse(options, args); diff --git a/spring-shell-docs/src/main/asciidoc/using-shell-options-arity.adoc b/spring-shell-docs/src/main/asciidoc/using-shell-options-arity.adoc index 5096caca..a25ed93d 100644 --- a/spring-shell-docs/src/main/asciidoc/using-shell-options-arity.adoc +++ b/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 <>. ==== [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. +---- +==== diff --git a/spring-shell-docs/src/main/asciidoc/using-shell-options-positional.adoc b/spring-shell-docs/src/main/asciidoc/using-shell-options-positional.adoc index 3aa1de47..b54563cd 100644 --- a/spring-shell-docs/src/main/asciidoc/using-shell-options-positional.adoc +++ b/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] +---- +==== diff --git a/spring-shell-docs/src/test/java/org/springframework/shell/docs/OptionSnippets.java b/spring-shell-docs/src/test/java/org/springframework/shell/docs/OptionSnippets.java index 2c472752..5b12925e 100644 --- a/spring-shell-docs/src/test/java/org/springframework/shell/docs/OptionSnippets.java +++ b/spring-shell-docs/src/test/java/org/springframework/shell/docs/OptionSnippets.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. @@ -15,6 +15,8 @@ */ package org.springframework.shell.docs; +import java.util.Arrays; + import org.springframework.shell.command.CommandRegistration; import org.springframework.shell.command.CommandRegistration.OptionArity; import org.springframework.shell.standard.ShellOption; @@ -144,6 +146,61 @@ public class OptionSnippets { .build(); // end::option-registration-arityints[] + // tag::option-registration-aritystrings-sample[] + CommandRegistration.builder() + .command("arity-errors") + .withOption() + .longNames("arg1") + .type(String[].class) + .required() + .arity(1, 2) + .and() + .withTarget() + .function(ctx -> { + String[] arg1 = ctx.getOptionValue("arg1"); + return "Hello " + Arrays.asList(arg1); + }) + .and() + .build(); + // end::option-registration-aritystrings-sample[] + + // tag::option-registration-aritystrings-position[] + CommandRegistration.builder() + .command("arity-strings-2") + .withOption() + .longNames("arg1") + .required() + .type(String[].class) + .arity(0, 2) + .position(0) + .and() + .withTarget() + .function(ctx -> { + String[] arg1 = ctx.getOptionValue("arg1"); + return "Hello " + Arrays.asList(arg1); + }) + .and() + .build(); + // end::option-registration-aritystrings-position[] + + // tag::option-registration-aritystrings-noposition[] + CommandRegistration.builder() + .command("arity-strings-1") + .withOption() + .longNames("arg1") + .required() + .type(String[].class) + .arity(0, 2) + .and() + .withTarget() + .function(ctx -> { + String[] arg1 = ctx.getOptionValue("arg1"); + return "Hello " + Arrays.asList(arg1); + }) + .and() + .build(); + // end::option-registration-aritystrings-noposition[] + // tag::option-registration-optional[] CommandRegistration.builder() .withOption() diff --git a/spring-shell-samples/src/main/java/org/springframework/shell/samples/e2e/ArityCommands.java b/spring-shell-samples/src/main/java/org/springframework/shell/samples/e2e/ArityCommands.java index 62413cd3..62388b42 100644 --- a/spring-shell-samples/src/main/java/org/springframework/shell/samples/e2e/ArityCommands.java +++ b/spring-shell-samples/src/main/java/org/springframework/shell/samples/e2e/ArityCommands.java @@ -75,8 +75,10 @@ public class ArityCommands extends BaseE2ECommands { .group(GROUP) .withOption() .longNames("arg1") + .required() .type(String[].class) .arity(0, 3) + .position(0) .and() .withTarget() .function(ctx -> {