Skip to content

Improved completion by allowing configuration of whether option values should be completed #1250

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
czpilar wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Improved completion by allowing configuration of whether option values should be completed #1250

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions spring-shell-core/src/main/java/org/springframework/shell/core/command/CommandOption.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
* @author Mahmoud Ben Hassine
*/
public record CommandOption(char shortName, @Nullable String longName, @Nullable String description,
@Nullable Boolean required, @Nullable String defaultValue, @Nullable String value) {
@Nullable Boolean required, @Nullable String defaultValue, @Nullable String value, boolean completion) {

public static Builder with() {
return new Builder();
Expand All @@ -46,6 +46,8 @@ public static class Builder {

private @Nullable String value;

private boolean completion = true;

public Builder shortName(char shortName) {
this.shortName = shortName;
return this;
Expand Down Expand Up @@ -76,8 +78,13 @@ public Builder value(String value) {
return this;
}

public Builder completion(boolean completion) {
this.completion = completion;
return this;
}

public CommandOption build() {
return new CommandOption(shortName, longName, description, required, defaultValue, value);
return new CommandOption(shortName, longName, description, required, defaultValue, value, completion);
}

}
Expand Down
6 changes: 6 additions & 0 deletions ...ng-shell-core/src/main/java/org/springframework/shell/core/command/annotation/Option.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,4 +62,10 @@
*/
String defaultValue() default "";

/**
* Indicates whether this option should be completed in the command completer.
* @return true if the option should be completed, defaults to true.
*/
boolean completion() default true;

}
2 changes: 2 additions & 0 deletions ...in/java/org/springframework/shell/core/command/annotation/support/CommandFactoryBean.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ private List<CommandOption> getCommandOptions() {
String description = optionAnnotation.description();
boolean required = optionAnnotation.required();
String defaultValue = optionAnnotation.defaultValue();
boolean completion = optionAnnotation.completion();
if (shortName == ' ' && longName.isEmpty()) {
throw new IllegalArgumentException(
"Either shortName or longName (or both) must be provided for option on parameter '"
Expand All @@ -133,6 +134,7 @@ private List<CommandOption> getCommandOptions() {
.description(description)
.required(required)
.defaultValue(defaultValue)
.completion(completion)
.build();
commandOptions.add(commandOption);
}
Expand Down
10 changes: 7 additions & 3 deletions spring-shell-jline/src/main/java/org/springframework/shell/jline/CommandCompleter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,14 @@ public void complete(LineReader reader, ParsedLine line, List<Candidate> candida
// add option completions for the command
for (CommandOption option : options) {
boolean present = isOptionPresent(line, option);
String separator = option.completion() ? "" : "=";
if (option.longName() != null && !present) {
candidates.add(new Candidate("--" + option.longName()));
candidates.add(new Candidate("--" + option.longName() + separator, "--" + option.longName(),
null, null, null, null, option.completion(), 0));
}
if (option.shortName() != ' ' && !present) {
candidates.add(new Candidate("-" + option.shortName()));
candidates.add(new Candidate("-" + option.shortName() + separator, "-" + option.shortName(),
null, null, null, null, option.completion(), 0));
}
}
}
Expand All @@ -59,7 +62,8 @@ public void complete(LineReader reader, ParsedLine line, List<Candidate> candida
commandByName, commandOption);
List<CompletionProposal> proposals = completionProvider.apply(context);
for (CompletionProposal proposal : proposals) {
candidates.add(new Candidate(proposal.value()));
candidates.add(new Candidate(proposal.value(), proposal.displayText(), proposal.category(),
proposal.description(), null, null, proposal.complete(), 0));
}
}
else {
Expand Down
68 changes: 50 additions & 18 deletions spring-shell-jline/src/test/java/org/springframework/shell/jline/CommandCompleterTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ else if ("last".equals(option.longName()) || 'l' == option.shortName()) {
options = Stream.of("Chan", "Noris");
}

return options.map(str -> prefix + str).map(CompletionProposal::new).toList();
return options.map(str -> new CompletionProposal(prefix + str).displayText(str)).toList();
};

@BeforeEach
Expand All @@ -85,8 +85,8 @@ private List<String> toCandidateNames(List<Candidate> candidates) {
}

@ParameterizedTest
@MethodSource("completeData")
public void testComplete(List<String> words, List<String> expectedValues) {
@MethodSource("completeWithCompletionData")
public void testCompleteWithCompletion(List<String> words, List<String> expectedValues) {
// given
when(command.getName()).thenReturn("hello");
when(command.getOptions())
Expand All @@ -106,49 +106,81 @@ public void testComplete(List<String> words, List<String> expectedValues) {
assertEquals(expectedValues, toCandidateNames(candidates));
}

static Stream<Arguments> completeData() {
static Stream<Arguments> completeWithCompletionData() {
return completeData("");
}

@ParameterizedTest
@MethodSource("completeWithoutCompletionData")
public void testCompleteWithoutCompletion(List<String> words, List<String> expectedValues) {
// given
when(command.getName()).thenReturn("hello");
when(command.getOptions())
.thenReturn(List.of(new CommandOption.Builder().longName("first").shortName('f').completion(false).build(),
new CommandOption.Builder().longName("last").shortName('l').completion(false).build()));

List<Candidate> candidates = new ArrayList<>();
ParsedLine line = mock(ParsedLine.class);
when(line.words()).thenReturn(words);
when(line.word()).thenReturn(words.get(words.size() - 1));
when(line.line()).thenReturn(String.join(" ", words));

// when
completer.complete(mock(LineReader.class), line, candidates);

// then
assertEquals(expectedValues, toCandidateNames(candidates));
}

static Stream<Arguments> completeWithoutCompletionData() {
return completeData("=");
}

static Stream<Arguments> completeData(String sep) {
return Stream.of(Arguments.of(List.of(""), List.of("hello")), Arguments.of(List.of("he"), List.of("hello")),
Arguments.of(List.of("he", ""), List.of()),

Arguments.of(List.of("hello"), List.of("--first", "--last", "-f", "-l")),
Arguments.of(List.of("hello", ""), List.of("--first", "--last", "-f", "-l")),
Arguments.of(List.of("hello"), List.of("--first" + sep, "--last" + sep, "-f" + sep, "-l" + sep)),
Arguments.of(List.of("hello", ""), List.of("--first" + sep, "--last" + sep, "-f" + sep, "-l" + sep)),

Arguments.of(List.of("hello", "--"), List.of("--first", "--last", "-f", "-l")),
Arguments.of(List.of("hello", "-"), List.of("--first", "--last", "-f", "-l")),
Arguments.of(List.of("hello", "--fi"), List.of("--first", "--last", "-f", "-l")),
Arguments.of(List.of("hello", "--la"), List.of("--first", "--last", "-f", "-l")),
Arguments.of(List.of("hello", "--"), List.of("--first" + sep, "--last" + sep, "-f" + sep, "-l" + sep)),
Arguments.of(List.of("hello", "-"), List.of("--first" + sep, "--last" + sep, "-f" + sep, "-l" + sep)),
Arguments.of(List.of("hello", "--fi"),
List.of("--first" + sep, "--last" + sep, "-f" + sep, "-l" + sep)),
Arguments.of(List.of("hello", "--la"),
List.of("--first" + sep, "--last" + sep, "-f" + sep, "-l" + sep)),

Arguments.of(List.of("hello", "-f"), List.of("-f=Mary", "-f=Paul", "-f=Peter")),
Arguments.of(List.of("hello", "-f="), List.of("-f=Mary", "-f=Paul", "-f=Peter")),
Arguments.of(List.of("hello", "-f=Pe"), List.of("-f=Mary", "-f=Paul", "-f=Peter")),
Arguments.of(List.of("hello", "-f=Pe", ""), List.of("--last", "-l")),
Arguments.of(List.of("hello", "-f=Pe", ""), List.of("--last" + sep, "-l" + sep)),

Arguments.of(List.of("hello", "--first"), List.of("--first=Mary", "--first=Paul", "--first=Peter")),
Arguments.of(List.of("hello", "--first="), List.of("--first=Mary", "--first=Paul", "--first=Peter")),
Arguments.of(List.of("hello", "--first=Pe"), List.of("--first=Mary", "--first=Paul", "--first=Peter")),
Arguments.of(List.of("hello", "--first=Pe", ""), List.of("--last", "-l")),
Arguments.of(List.of("hello", "--first=Pe", ""), List.of("--last" + sep, "-l" + sep)),

Arguments.of(List.of("hello", "-f", ""), List.of("Mary", "Paul", "Peter")),
Arguments.of(List.of("hello", "--first", ""), List.of("Mary", "Paul", "Peter")),

Arguments.of(List.of("hello", "-f", "Pe"), List.of("--last", "-l")),
Arguments.of(List.of("hello", "--first", "Pe"), List.of("--last", "-l")),
Arguments.of(List.of("hello", "-f", "Pe"), List.of("--last" + sep, "-l" + sep)),
Arguments.of(List.of("hello", "--first", "Pe"), List.of("--last" + sep, "-l" + sep)),

Arguments.of(List.of("hello", "-l"), List.of("-l=Chan", "-l=Noris")),
Arguments.of(List.of("hello", "-l="), List.of("-l=Chan", "-l=Noris")),
Arguments.of(List.of("hello", "-l=No"), List.of("-l=Chan", "-l=Noris")),
Arguments.of(List.of("hello", "-l=No", ""), List.of("--first", "-f")),
Arguments.of(List.of("hello", "-l=No", ""), List.of("--first" + sep, "-f" + sep)),

Arguments.of(List.of("hello", "--last"), List.of("--last=Chan", "--last=Noris")),
Arguments.of(List.of("hello", "--last="), List.of("--last=Chan", "--last=Noris")),
Arguments.of(List.of("hello", "--last=No"), List.of("--last=Chan", "--last=Noris")),
Arguments.of(List.of("hello", "--last=No", ""), List.of("--first", "-f")),
Arguments.of(List.of("hello", "--last=No", ""), List.of("--first" + sep, "-f" + sep)),

Arguments.of(List.of("hello", "-l", ""), List.of("Chan", "Noris")),
Arguments.of(List.of("hello", "--last", ""), List.of("Chan", "Noris")),

Arguments.of(List.of("hello", "-l", "No"), List.of("--first", "-f")),
Arguments.of(List.of("hello", "--last", "No"), List.of("--first", "-f")),
Arguments.of(List.of("hello", "-l", "No"), List.of("--first" + sep, "-f" + sep)),
Arguments.of(List.of("hello", "--last", "No"), List.of("--first" + sep, "-f" + sep)),

Arguments.of(List.of("hello", "--first", "Paul", "--last", "Noris"), List.of()),
Arguments.of(List.of("hello", "--first", "Paul", "-l", "Noris"), List.of()),
Expand Down