From c125c46010aaf29164e7e695549704f1757b0f18 Mon Sep 17 00:00:00 2001 From: Janne Valkealahti Date: Wed, 31 May 2023 17:28:21 +0100 Subject: [PATCH] Add short options into valid tokens - Add short options as valid tokens in a `CommandModel` so that Lexer create tokens with accurate info and doesn't then cascade this issue in Ast and Parser. - Previously with a command `command -a aaa -b bbb` tokenisation resulted `COMMAND OPTION ARGUMENT ARGUMENT ARGUMENT` with multiple short options while it should have been `COMMAND OPTION ARGUMENT OPTION ARGUMENT`. - Relates #757 --- .../shell/command/parser/CommandModel.java | 3 + .../command/parser/AbstractParsingTests.java | 41 ++++++++ .../shell/command/parser/AstTests.java | 96 +++++++++++++++++++ .../shell/command/parser/LexerTests.java | 64 +++++++++++++ .../shell/command/parser/ParserTests.java | 72 ++++++++++++++ 5 files changed, 276 insertions(+) diff --git a/spring-shell-core/src/main/java/org/springframework/shell/command/parser/CommandModel.java b/spring-shell-core/src/main/java/org/springframework/shell/command/parser/CommandModel.java index cff61447..8a65e51e 100644 --- a/spring-shell-core/src/main/java/org/springframework/shell/command/parser/CommandModel.java +++ b/spring-shell-core/src/main/java/org/springframework/shell/command/parser/CommandModel.java @@ -196,6 +196,9 @@ public class CommandModel { for (String longName : commandOption.getLongNames()) { tokens.put("--" + longName, new Token(longName, TokenType.OPTION)); } + for (Character shortName : commandOption.getShortNames()) { + tokens.put("-" + shortName, new Token(shortName.toString(), TokenType.OPTION)); + } }); } return tokens; 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 a21ecd53..4c2f9ca2 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 @@ -110,6 +110,19 @@ abstract class AbstractParsingTests { .and() .build(); + static final CommandRegistration ROOT3_OPTION_ARG1_ARG2 = CommandRegistration.builder() + .command("root3") + .withOption() + .longNames("arg1") + .and() + .withOption() + .longNames("arg2") + .and() + .withTarget() + .consumer(ctx -> {}) + .and() + .build(); + static final CommandRegistration ROOT3_SHORT_OPTION_A = CommandRegistration.builder() .command("root3") .withOption() @@ -120,6 +133,34 @@ abstract class AbstractParsingTests { .and() .build(); + static final CommandRegistration ROOT3_SHORT_OPTION_A_B = CommandRegistration.builder() + .command("root3") + .withOption() + .shortNames('a') + .and() + .withOption() + .shortNames('b') + .and() + .withTarget() + .consumer(ctx -> {}) + .and() + .build(); + + static final CommandRegistration ROOT3_SHORT_OPTION_A_B_REQUIRED = CommandRegistration.builder() + .command("root3") + .withOption() + .shortNames('a') + .required() + .and() + .withOption() + .required() + .shortNames('b') + .and() + .withTarget() + .consumer(ctx -> {}) + .and() + .build(); + static final CommandRegistration ROOT4 = CommandRegistration.builder() .command("root4") .withOption() diff --git a/spring-shell-core/src/test/java/org/springframework/shell/command/parser/AstTests.java b/spring-shell-core/src/test/java/org/springframework/shell/command/parser/AstTests.java index 4eee158c..79832245 100644 --- a/spring-shell-core/src/test/java/org/springframework/shell/command/parser/AstTests.java +++ b/spring-shell-core/src/test/java/org/springframework/shell/command/parser/AstTests.java @@ -137,6 +137,102 @@ class AstTests extends AbstractParsingTests { }); } + @Test + void createsOptionNodesWithTwoOptionArg() { + register(ROOT3); + Token root3 = token("root3", TokenType.COMMAND, 0); + Token arg1 = token("--arg1", TokenType.OPTION, 1); + Token value1 = token("value1", TokenType.ARGUMENT, 2); + Token arg2 = token("--arg2", TokenType.OPTION, 3); + Token value2 = token("value2", TokenType.ARGUMENT, 4); + AstResult result = ast(root3, arg1, value1, arg2, value2); + + assertThat(result).isNotNull(); + assertThat(result.nonterminalNodes()).hasSize(1); + assertThat(result.nonterminalNodes().get(0)).isInstanceOf(CommandNode.class); + assertThat(result.nonterminalNodes().get(0)).satisfies(n -> { + CommandNode cn = (CommandNode)n; + assertThat(cn.getCommand()).isEqualTo("root3"); + assertThat(cn.getChildren()).hasSize(2); + assertThat(cn.getChildren()) + .filteredOn(on -> on instanceof OptionNode) + .extracting(on -> { + return ((OptionNode)on).getName(); + }) + .containsExactly("--arg1", "--arg2"); + OptionNode on1 = (OptionNode) cn.getChildren().get(0); + assertThat(on1.getChildren()).hasSize(1); + OptionArgumentNode oan1 = (OptionArgumentNode) on1.getChildren().get(0); + assertThat(oan1.getValue()).isEqualTo("value1"); + OptionNode on2 = (OptionNode) cn.getChildren().get(1); + assertThat(on2.getChildren()).hasSize(1); + OptionArgumentNode oan2 = (OptionArgumentNode) on2.getChildren().get(0); + assertThat(oan2.getValue()).isEqualTo("value2"); + }); + } + + @Test + void createsOptionNodeWithShortOptionArg() { + register(ROOT3_SHORT_OPTION_A); + Token root3 = token("root3", TokenType.COMMAND, 0); + Token arg1 = token("-a", TokenType.OPTION, 1); + Token value1 = token("value1", TokenType.ARGUMENT, 2); + AstResult result = ast(root3, arg1, value1); + + assertThat(result).isNotNull(); + assertThat(result.nonterminalNodes()).hasSize(1); + assertThat(result.nonterminalNodes().get(0)).isInstanceOf(CommandNode.class); + assertThat(result.nonterminalNodes().get(0)).satisfies(n -> { + CommandNode cn = (CommandNode)n; + assertThat(cn.getCommand()).isEqualTo("root3"); + assertThat(cn.getChildren()).hasSize(1); + assertThat(cn.getChildren()) + .filteredOn(on -> on instanceof OptionNode) + .extracting(on -> { + return ((OptionNode)on).getName(); + }) + .containsExactly("-a"); + OptionNode on = (OptionNode) cn.getChildren().get(0); + assertThat(on.getChildren()).hasSize(1); + OptionArgumentNode oan = (OptionArgumentNode) on.getChildren().get(0); + assertThat(oan.getValue()).isEqualTo("value1"); + }); + } + + @Test + void createsOptionNodesWithTwoShortOptionArg() { + register(ROOT3_SHORT_OPTION_A_B); + Token root3 = token("root3", TokenType.COMMAND, 0); + Token arg1 = token("-a", TokenType.OPTION, 1); + Token value1 = token("value1", TokenType.ARGUMENT, 2); + Token arg2 = token("-b", TokenType.OPTION, 3); + Token value2 = token("value2", TokenType.ARGUMENT, 4); + AstResult result = ast(root3, arg1, value1, arg2, value2); + + assertThat(result).isNotNull(); + assertThat(result.nonterminalNodes()).hasSize(1); + assertThat(result.nonterminalNodes().get(0)).isInstanceOf(CommandNode.class); + assertThat(result.nonterminalNodes().get(0)).satisfies(n -> { + CommandNode cn = (CommandNode)n; + assertThat(cn.getCommand()).isEqualTo("root3"); + assertThat(cn.getChildren()).hasSize(2); + assertThat(cn.getChildren()) + .filteredOn(on -> on instanceof OptionNode) + .extracting(on -> { + return ((OptionNode)on).getName(); + }) + .containsExactly("-a", "-b"); + OptionNode on1 = (OptionNode) cn.getChildren().get(0); + assertThat(on1.getChildren()).hasSize(1); + OptionArgumentNode oan1 = (OptionArgumentNode) on1.getChildren().get(0); + assertThat(oan1.getValue()).isEqualTo("value1"); + OptionNode on2 = (OptionNode) cn.getChildren().get(1); + assertThat(on2.getChildren()).hasSize(1); + OptionArgumentNode oan2 = (OptionArgumentNode) on2.getChildren().get(0); + assertThat(oan2.getValue()).isEqualTo("value2"); + }); + } + @Test void createOptionNodesWhenNoOptionArguments() { register(ROOT3); diff --git a/spring-shell-core/src/test/java/org/springframework/shell/command/parser/LexerTests.java b/spring-shell-core/src/test/java/org/springframework/shell/command/parser/LexerTests.java index 2b7ec0a2..94fb79e4 100644 --- a/spring-shell-core/src/test/java/org/springframework/shell/command/parser/LexerTests.java +++ b/spring-shell-core/src/test/java/org/springframework/shell/command/parser/LexerTests.java @@ -336,6 +336,70 @@ class LexerTests extends AbstractParsingTests { }); } + @Test + void shortOptionWithValuesFromRoot() { + register(ROOT3_SHORT_OPTION_A); + List tokens = tokenize("root3", "-a", "value1"); + + assertThat(tokens).satisfiesExactly( + token -> { + ParserAssertions.assertThat(token) + .isType(TokenType.COMMAND) + .hasPosition(0) + .hasValue("root3"); + }, + token -> { + ParserAssertions.assertThat(token) + .isType(TokenType.OPTION) + .hasPosition(1) + .hasValue("-a"); + }, + token -> { + ParserAssertions.assertThat(token) + .isType(TokenType.ARGUMENT) + .hasPosition(2) + .hasValue("value1"); + }); + } + + @Test + void shortOptionsWithValuesFromRoot() { + register(ROOT3_SHORT_OPTION_A_B); + List tokens = tokenize("root3", "-a", "value1", "-b", "value2"); + + assertThat(tokens).satisfiesExactly( + token -> { + ParserAssertions.assertThat(token) + .isType(TokenType.COMMAND) + .hasPosition(0) + .hasValue("root3"); + }, + token -> { + ParserAssertions.assertThat(token) + .isType(TokenType.OPTION) + .hasPosition(1) + .hasValue("-a"); + }, + token -> { + ParserAssertions.assertThat(token) + .isType(TokenType.ARGUMENT) + .hasPosition(2) + .hasValue("value1"); + }, + token -> { + ParserAssertions.assertThat(token) + .isType(TokenType.OPTION) + .hasPosition(3) + .hasValue("-b"); + }, + token -> { + ParserAssertions.assertThat(token) + .isType(TokenType.ARGUMENT) + .hasPosition(4) + .hasValue("value2"); + }); + } + @Test void optionValueFromRoot() { register(ROOT3); 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 ed4fe67a..d31934ac 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 @@ -251,6 +251,26 @@ class ParserTests extends AbstractParsingTests { ); assertThat(result.messageResults()).isEmpty(); } + + @Test + void shouldFindTwoLongOptionArgument() { + register(ROOT3_OPTION_ARG1_ARG2); + ParseResult result = parse("root3", "--arg1", "value1", "--arg2", "value2"); + assertThat(result).isNotNull(); + assertThat(result.commandRegistration()).isNotNull(); + assertThat(result.optionResults()).isNotEmpty(); + assertThat(result.optionResults()).satisfiesExactly( + r -> { + assertThat(r.option().getLongNames()).isEqualTo(new String[] { "arg1" }); + assertThat(r.value()).isEqualTo("value1"); + }, + r -> { + assertThat(r.option().getLongNames()).isEqualTo(new String[] { "arg2" }); + assertThat(r.value()).isEqualTo("value2"); + } + ); + assertThat(result.messageResults()).isEmpty(); + } } @Nested @@ -263,6 +283,58 @@ class ParserTests extends AbstractParsingTests { assertThat(result).isNotNull(); assertThat(result.commandRegistration()).isNotNull(); assertThat(result.optionResults()).isNotEmpty(); + assertThat(result.optionResults()).satisfiesExactly( + r -> { + assertThat(r.option().getShortNames()).isEqualTo(new Character[] { 'a' }); + assertThat(r.value()).isNull(); + } + ); + assertThat(result.messageResults()).isEmpty(); + } + + @Test + void shouldFindShortOptionWithArg() { + register(ROOT3_SHORT_OPTION_A); + ParseResult result = parse("root3", "-a", "aaa"); + assertThat(result).isNotNull(); + assertThat(result.commandRegistration()).isNotNull(); + assertThat(result.optionResults()).isNotEmpty(); + assertThat(result.optionResults()).satisfiesExactly( + r -> { + assertThat(r.option().getShortNames()).isEqualTo(new Character[] { 'a' }); + assertThat(r.value()).isEqualTo("aaa"); + } + ); + assertThat(result.messageResults()).isEmpty(); + } + + @Test + void shouldFindShortOptions() { + register(ROOT3_SHORT_OPTION_A_B); + ParseResult result = parse("root3", "-a", "aaa", "-b", "bbb"); + assertThat(result).isNotNull(); + assertThat(result.commandRegistration()).isNotNull(); + assertThat(result.optionResults()).isNotEmpty(); + assertThat(result.optionResults()).satisfiesExactly( + r -> { + assertThat(r.option().getShortNames()).isEqualTo(new Character[] { 'a' }); + assertThat(r.value()).isEqualTo("aaa"); + }, + r -> { + assertThat(r.option().getShortNames()).isEqualTo(new Character[] { 'b' }); + assertThat(r.value()).isEqualTo("bbb"); + } + ); + assertThat(result.messageResults()).isEmpty(); + } + + @Test + void shouldFindShortOptionsRequired() { + register(ROOT3_SHORT_OPTION_A_B_REQUIRED); + ParseResult result = parse("root3", "-a", "aaa", "-b", "bbb"); + assertThat(result).isNotNull(); + assertThat(result.commandRegistration()).isNotNull(); + assertThat(result.optionResults()).isNotEmpty(); assertThat(result.messageResults()).isEmpty(); }