Skip to content

Commit 809e296

Browse files
committed
Handle arg looking options better
- Now lexing better with valid options - Only report unrecognised option with double dash as current parser don't have structure to do deeper analysis. - Fixes #651
1 parent 3ce7afa commit 809e296

File tree

2 files changed

+47
-4
lines changed

2 files changed

+47
-4
lines changed

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

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
import java.util.Comparator;
2323
import java.util.Deque;
2424
import java.util.List;
25+
import java.util.Set;
2526
import java.util.stream.Collectors;
2627
import java.util.stream.Stream;
2728

@@ -213,7 +214,15 @@ public CommandParserResults parse(List<CommandOption> options, String[] args) {
213214
.filter(o -> o.isRequired())
214215
.collect(Collectors.toList());
215216

216-
Lexer lexer = new Lexer(args);
217+
Set<String> splitValidValues = options.stream()
218+
.flatMap(o -> {
219+
Stream<String> longs = Stream.of(o.getLongNames()).map(l -> "--" + l);
220+
Stream<String> shorts = Stream.of(o.getShortNames()).map(s -> "-"+ Character.toString(s));
221+
return Stream.concat(longs, shorts);
222+
})
223+
.collect(Collectors.toSet());
224+
225+
Lexer lexer = new Lexer(args, splitValidValues);
217226
List<List<String>> lexerResults = lexer.visit();
218227
Parser parser = new Parser();
219228
ParserResults parserResults = parser.visit(lexerResults, options);
@@ -228,7 +237,7 @@ public CommandParserResults parse(List<CommandOption> options, String[] args) {
228237
}
229238
else {
230239
for (String arg : pr.args) {
231-
if (arg.startsWith("-")) {
240+
if (arg.startsWith("--")) {
232241
errors.add(UnrecognisedOptionException.of(String.format("Unrecognised option '%s'", arg),
233242
arg));
234243
}
@@ -520,11 +529,13 @@ private class ConvertArgumentsHolder {
520529
*/
521530
private static class Lexer {
522531
private final String[] args;
523-
Lexer(String[] args) {
532+
private final Set<String> splitValidValues;
533+
Lexer(String[] args, Set<String> splitValues) {
524534
this.args = args;
535+
this.splitValidValues = splitValues;
525536
}
526537
List<List<String>> visit() {
527-
return Utils.split(args, t -> t.startsWith("-"));
538+
return Utils.split(args, t -> splitValidValues.contains(t));
528539
}
529540
}
530541
}

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

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,10 @@
2626
import org.springframework.core.convert.ConversionService;
2727
import org.springframework.core.convert.support.DefaultConversionService;
2828
import org.springframework.shell.command.CommandParser.CommandParserResults;
29+
import org.springframework.shell.command.CommandParser.MissingOptionException;
2930
import org.springframework.shell.command.CommandParser.NotEnoughArgumentsOptionException;
3031
import org.springframework.shell.command.CommandParser.TooManyArgumentsOptionException;
32+
import org.springframework.shell.command.CommandParser.UnrecognisedOptionException;
3133

3234
import static org.assertj.core.api.Assertions.assertThat;
3335

@@ -496,6 +498,11 @@ public void testNotDefinedLongOptionWithoutOptions() {
496498
CommandParserResults results = parser.parse(options, args);
497499
assertThat(results.results()).hasSize(0);
498500
assertThat(results.errors()).hasSize(1);
501+
assertThat(results.errors()).satisfiesExactly(
502+
e -> {
503+
assertThat(e).isInstanceOf(UnrecognisedOptionException.class);
504+
}
505+
);
499506
assertThat(results.positional()).hasSize(1);
500507
}
501508

@@ -508,6 +515,11 @@ public void testNotDefinedLongOptionWithOptionalOption() {
508515
CommandParserResults results = parser.parse(options, args);
509516
assertThat(results.results()).hasSize(1);
510517
assertThat(results.errors()).hasSize(1);
518+
assertThat(results.errors()).satisfiesExactly(
519+
e -> {
520+
assertThat(e).isInstanceOf(UnrecognisedOptionException.class);
521+
}
522+
);
511523
assertThat(results.positional()).hasSize(1);
512524
}
513525

@@ -520,9 +532,29 @@ public void testNotDefinedLongOptionWithRequiredOption() {
520532
CommandParserResults results = parser.parse(options, args);
521533
assertThat(results.results()).hasSize(0);
522534
assertThat(results.errors()).hasSize(2);
535+
assertThat(results.errors()).satisfiesExactly(
536+
e -> {
537+
assertThat(e).isInstanceOf(UnrecognisedOptionException.class);
538+
},
539+
e -> {
540+
assertThat(e).isInstanceOf(MissingOptionException.class);
541+
}
542+
);
523543
assertThat(results.positional()).hasSize(1);
524544
}
525545

546+
@Test
547+
public void testDashOptionValueDoNotError() {
548+
// gh-651
549+
CommandOption option1 = longOption("arg1");
550+
List<CommandOption> options = Arrays.asList(option1);
551+
String[] args = new String[]{"--arg1", "-1"};
552+
CommandParserResults results = parser.parse(options, args);
553+
assertThat(results.results()).hasSize(1);
554+
assertThat(results.errors()).hasSize(0);
555+
assertThat(results.positional()).hasSize(0);
556+
}
557+
526558
@Test
527559
public void testPositionDoesNotAffectRequiredErrorWithOtherErrors() {
528560
// gh-601

0 commit comments

Comments
 (0)