From 64abc1da2274864030eb81ca73aa54375dd3d058 Mon Sep 17 00:00:00 2001 From: Janne Valkealahti Date: Wed, 5 Apr 2023 13:42:22 +0100 Subject: [PATCH] Fix positional args with new parser - Better alignment for use of positional args without using an option. - Should work similarly with old parsing framework. - Change position where missing options are checked. - Fixes #701 --- .../shell/command/parser/Parser.java | 32 ++++++++++--- .../command/parser/AbstractParsingTests.java | 29 +++++++++++ .../shell/command/parser/ParserTests.java | 48 ++++++++++++++++--- .../shell/samples/e2e/OptionTypeCommands.java | 1 + .../samples/e2e/OptionTypeCommandsTests.java | 7 +++ 5 files changed, 104 insertions(+), 13 deletions(-) diff --git a/spring-shell-core/src/main/java/org/springframework/shell/command/parser/Parser.java b/spring-shell-core/src/main/java/org/springframework/shell/command/parser/Parser.java index cf65211f..4a98b426 100644 --- a/spring-shell-core/src/main/java/org/springframework/shell/command/parser/Parser.java +++ b/spring-shell-core/src/main/java/org/springframework/shell/command/parser/Parser.java @@ -17,6 +17,7 @@ package org.springframework.shell.command.parser; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.Comparator; import java.util.HashSet; import java.util.List; @@ -162,7 +163,6 @@ public interface Parser { List messageResults = new ArrayList<>(); if (registration != null) { messageResults.addAll(commonMessageResults); - messageResults.addAll(validateOptionNotMissing(registration)); messageResults.addAll(validateOptionIsValid(registration)); @@ -182,7 +182,6 @@ public interface Parser { List optionsForArguments = registration.getOptions().stream() .filter(o -> !resolvedOptions1.contains(o)) .filter(o -> o.getPosition() > -1) - .filter(o -> o.getArityMin() > -1) .sorted(Comparator.comparingInt(o -> o.getPosition())) .collect(Collectors.toList()); @@ -193,7 +192,11 @@ public interface Parser { int i = 0; for (CommandOption o : optionsForArguments) { - int j = i + o.getArityMax(); + int aMax = o.getArityMax(); + if (aMax < 0) { + aMax = optionsForArguments.size() == 1 ? Integer.MAX_VALUE : 1; + } + int j = i + aMax; j = Math.min(argumentValues.size(), j); List asdf = argumentValues.subList(i, j); @@ -211,6 +214,8 @@ public interface Parser { i = j; } + // can only validate after optionResults has been populated + messageResults.addAll(validateOptionNotMissing(registration)); } return new ParseResult(registration, optionResults, argumentResults, messageResults, directiveResults); @@ -369,11 +374,24 @@ public interface Parser { .filter(o -> o.isRequired()) .collect(Collectors.toCollection(() -> new HashSet<>())); - optionResults.stream().map(or -> or.option()).forEach(o -> { - requiredOptions.remove(o); - }); + List argumentResultValues = argumentResults.stream().map(ar -> ar.value).collect(Collectors.toList()); + optionResults.stream() + .filter(or -> or.value() != null) + .map(or -> or.option()) + .forEach(o -> { + requiredOptions.remove(o); + }); + Set requiredOptions2 = requiredOptions.stream() + .filter(o -> { + if (argumentResultValues.isEmpty()) { + return true; + } + List longNames = Arrays.asList(o.getLongNames()); + return !Collections.disjoint(argumentResultValues, longNames); + }) + .collect(Collectors.toSet()); - return requiredOptions.stream() + return requiredOptions2.stream() .map(o -> { String ln = o.getLongNames() != null ? Stream.of(o.getLongNames()).collect(Collectors.joining(",")) diff --git a/spring-shell-core/src/test/java/org/springframework/shell/command/parser/AbstractParsingTests.java b/spring-shell-core/src/test/java/org/springframework/shell/command/parser/AbstractParsingTests.java index 85522808..a21ecd53 100644 --- a/spring-shell-core/src/test/java/org/springframework/shell/command/parser/AbstractParsingTests.java +++ b/spring-shell-core/src/test/java/org/springframework/shell/command/parser/AbstractParsingTests.java @@ -181,6 +181,35 @@ abstract class AbstractParsingTests { .and() .build(); + static final CommandRegistration ROOT7_POSITIONAL_ONE_ARG_STRING = CommandRegistration.builder() + .command("root7") + .withOption() + .longNames("arg1") + .type(String.class) + .position(0) + .and() + .withTarget() + .consumer(ctx -> {}) + .and() + .build(); + + static final CommandRegistration ROOT7_POSITIONAL_TWO_ARG_STRING = CommandRegistration.builder() + .command("root7") + .withOption() + .longNames("arg1") + .type(String.class) + .position(0) + .and() + .withOption() + .longNames("arg2") + .type(String.class) + .position(1) + .and() + .withTarget() + .consumer(ctx -> {}) + .and() + .build(); + Map registrations = new HashMap<>(); @BeforeEach diff --git a/spring-shell-core/src/test/java/org/springframework/shell/command/parser/ParserTests.java b/spring-shell-core/src/test/java/org/springframework/shell/command/parser/ParserTests.java index 3a112df5..ed4fe67a 100644 --- a/spring-shell-core/src/test/java/org/springframework/shell/command/parser/ParserTests.java +++ b/spring-shell-core/src/test/java/org/springframework/shell/command/parser/ParserTests.java @@ -136,14 +136,17 @@ class ParserTests extends AbstractParsingTests { @Test void shouldHaveErrorResult2() { register(ROOT4); - // ParseResult result = parse("root4", "--arg1", "value1", "--arg2", "value2"); ParseResult result = parse("root4", "--arg1", "--arg2"); assertThat(result).isNotNull(); - assertThat(result.messageResults()).satisfiesExactly(message -> { - ParserAssertions.assertThat(message.parserMessage()).hasCode(2001).hasType(ParserMessage.Type.ERROR); - }); - // "101E:(pos 0): Unrecognised option '--arg2'" - // assertThat(result.messageResults().get(0).getMessage()).contains("xxx"); + + assertThat(result.messageResults()).satisfiesExactlyInAnyOrder( + message -> { + ParserAssertions.assertThat(message.parserMessage()).hasCode(2000).hasType(ParserMessage.Type.ERROR); + }, + message -> { + ParserAssertions.assertThat(message.parserMessage()).hasCode(2001).hasType(ParserMessage.Type.ERROR); + } + ); } @Test @@ -339,4 +342,37 @@ class ParserTests extends AbstractParsingTests { ); } } + + @Nested + class Positional { + + @Test + void shouldGetPositionalArgWhenOneAsString() { + register(ROOT7_POSITIONAL_ONE_ARG_STRING); + ParseResult result = parse("root7", "a"); + assertThat(result.argumentResults()).satisfiesExactly( + r -> { + assertThat(r.value()).isEqualTo("a"); + assertThat(r.position()).isEqualTo(0); + } + ); + } + + @Test + void shouldGetPositionalArgWhenTwoAsString() { + register(ROOT7_POSITIONAL_TWO_ARG_STRING); + ParseResult result = parse("root7", "a", "b"); + assertThat(result.argumentResults()).satisfiesExactly( + r -> { + assertThat(r.value()).isEqualTo("a"); + assertThat(r.position()).isEqualTo(0); + }, + r -> { + assertThat(r.value()).isEqualTo("b"); + assertThat(r.position()).isEqualTo(1); + } + ); + } + } + } diff --git a/spring-shell-samples/src/main/java/org/springframework/shell/samples/e2e/OptionTypeCommands.java b/spring-shell-samples/src/main/java/org/springframework/shell/samples/e2e/OptionTypeCommands.java index d53be36d..68dd11dc 100644 --- a/spring-shell-samples/src/main/java/org/springframework/shell/samples/e2e/OptionTypeCommands.java +++ b/spring-shell-samples/src/main/java/org/springframework/shell/samples/e2e/OptionTypeCommands.java @@ -216,6 +216,7 @@ public class OptionTypeCommands { .withOption() .longNames("arg1") .type(String.class) + .position(0) .required() .and() .withTarget() diff --git a/spring-shell-samples/src/test/java/org/springframework/shell/samples/e2e/OptionTypeCommandsTests.java b/spring-shell-samples/src/test/java/org/springframework/shell/samples/e2e/OptionTypeCommandsTests.java index 8e418253..e27b8c3d 100644 --- a/spring-shell-samples/src/test/java/org/springframework/shell/samples/e2e/OptionTypeCommandsTests.java +++ b/spring-shell-samples/src/test/java/org/springframework/shell/samples/e2e/OptionTypeCommandsTests.java @@ -36,6 +36,13 @@ class OptionTypeCommandsTests extends AbstractSampleTests { assertScreenContainsText(session, "Hello hi"); } + @ParameterizedTest + @E2ESource(command = "option-type-string hi") + void optionTypeStringPositional(String command, boolean interactive) { + BaseShellSession session = createSession(command, interactive); + assertScreenContainsText(session, "Hello hi"); + } + @ParameterizedTest @E2ESource(command = "option-type-boolean", reg = false) void optionTypeBooleanWithAnno(String command, boolean interactive) {