valepakh commented on code in PR #1363: URL: https://github.com/apache/ignite-3/pull/1363#discussion_r1030389587
########## modules/cli/src/main/java/org/apache/ignite/internal/cli/core/repl/completer/CompleterConf.java: ########## @@ -0,0 +1,146 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal.cli.core.repl.completer; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.Set; +import java.util.stream.Collectors; +import java.util.stream.Stream; +import org.apache.ignite.internal.cli.commands.Options; + +/** + * Configuration for Dynamic completer. It declares for what command and option the compliter could be applied. + * Also supports explicit declaration of options after those the compliter should not be applied. + */ +public class CompleterConf { + + private final List<String[]> commands; + + private final String[] enableOptions; + + private final String[] disableOptions; + + private final boolean exclusiveEnableOptions; + + private CompleterConf(List<String[]> commands, String[] enableOptions, String[] disableOptions, boolean exclusiveEnableOptions) { + if (commands == null) { + throw new IllegalArgumentException("commands must not be null"); + } + + this.commands = commands; + this.enableOptions = enableOptions; + this.disableOptions = disableOptions; + this.exclusiveEnableOptions = exclusiveEnableOptions; + } + + public static CompleterConf everytime() { + return builder().build(); + } + + public static CompleterConf forCommand(String... words) { + return builder().command(words).build(); + } + + public static CompleterConfBuilder builder() { + return new CompleterConfBuilder(); + } + + public List<String[]> commands() { + return commands; + } + + public Set<String> enableOptions() { + return Set.of(enableOptions); + } + + public Set<String> disableOptions() { + return Set.of(disableOptions); + } + + public boolean commandSpecific() { + return !commands.isEmpty(); + } + + public boolean hasEnableOptions() { + return enableOptions != null; + } + + public boolean hasDisableOptions() { + return disableOptions != null; + } + + public boolean isExclusiveEnableOptions() { + return exclusiveEnableOptions; + } + + /** Builder for {@link CompleterConf}. */ + public static class CompleterConfBuilder { + private final List<String[]> command = new ArrayList<>(); + + private String[] enableOptions; + + private String[] disableOptions; Review Comment: These arrays are used only to wrap them to set in the CompleterConf, maybe it would be better to use sets here as well and skip some of the Arrays.copyOf in the builder methods? ########## modules/cli/src/main/java/org/apache/ignite/internal/cli/core/repl/completer/DynamicCompleterFilter.java: ########## @@ -43,25 +49,31 @@ private static boolean optionTyped(String[] words) { @Override public String[] filter(String[] words, String[] candidates) { + List<String> notOptionsCandidates = Arrays.stream(candidates) + .filter(candidate -> !candidate.startsWith("-")) + .collect(Collectors.toList()); + + if (!notOptionsCandidates.isEmpty()) { + return notOptionsCandidates.toArray(String[]::new); + } + return Arrays.stream(candidates) .filter(candidate -> filterClusterUrl(words, candidate)) - .filter(candidate -> filterHelp(words, candidate)) + .filter(candidate -> filterCommonOptions(words, candidate)) .toArray(String[]::new); } - private boolean filterHelp(String[] words, String candidate) { - if (optionTyped(words)) { - return true; - } - - return !(candidate.equals("--help") || candidate.equals("-h")); + private boolean filterCommonOptions(String[] words, String candidate) { + return optionTyped(words) + || !(HELP_OPTION.equals(candidate) + || HELP_OPTION_SHORT.equals(candidate) + || VERBOSE_OPTION_SHORT.equals(candidate) + || VERBOSE_OPTION.equals(candidate)); } private boolean filterClusterUrl(String[] words, String candidate) { - if (optionTyped(words)) { - return true; - } + return optionTyped(words) || !session.isConnectedToNode() || (!candidate.equals(CLUSTER_URL_OPTION) && !candidate.equals( + NODE_URL_OPTION)); Review Comment: Weird line break, maybe break before last `||`? ########## modules/cli/src/test/java/org/apache/ignite/internal/cli/core/repl/completer/DynamicCompleterRegistryTest.java: ########## @@ -45,28 +54,30 @@ void setUp() { } @Test - void findsCompletersRegisteredWithPredicate() { + void findsCompleterRegisteredWithMultyCommand() { // Given - registry.register(words -> words[0].equals("command1"), completer1); - registry.register(words -> words[0].equals("command2"), completer2); - registry.register(words -> words[0].equals("command2"), completer3); - - // When find completers for "command1" - List<DynamicCompleter> completers = registry.findCompleters(new String[]{"command1"}); + registry.register( + CompleterConf.builder() + .command("command1") + .command("command2").build(), + (words) -> completer1 + ); // Then - assertThat(completers, containsInAnyOrder(completer1)); + assertThat(registry.findCompleters(words("command1")), containsInAnyOrder(completer1)); + // And + assertThat(registry.findCompleters(words("command2")), containsInAnyOrder(completer1)); } @Test void findsCompletersRegisteredWithStaticCommand() { // Given - registry.register(new String[]{"command1", "subcommand1"}, completer1); - registry.register(new String[]{"command1", "subcommand1"}, completer2); - registry.register(new String[]{"command2"}, completer3); + registry.register(CompleterConf.forCommand("command1", "subcommand1"), (words) -> completer1); + registry.register(CompleterConf.forCommand("command1", "subcommand1"), (words) -> completer2); + registry.register(CompleterConf.forCommand("command2"), (words) -> completer3); Review Comment: ```suggestion registry.register(CompleterConf.forCommand("command1", "subcommand1"), words -> completer1); registry.register(CompleterConf.forCommand("command1", "subcommand1"), words -> completer2); registry.register(CompleterConf.forCommand("command2"), words -> completer3); ``` And so on. ########## modules/cli/src/main/java/org/apache/ignite/internal/cli/core/repl/completer/CompleterConf.java: ########## @@ -0,0 +1,146 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal.cli.core.repl.completer; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.Set; +import java.util.stream.Collectors; +import java.util.stream.Stream; +import org.apache.ignite.internal.cli.commands.Options; + +/** + * Configuration for Dynamic completer. It declares for what command and option the compliter could be applied. + * Also supports explicit declaration of options after those the compliter should not be applied. Review Comment: ```suggestion * Configuration for dynamic completer. It declares for what command and option the completer could be applied. * Also supports explicit declaration of options after which the completer should not be applied. ``` ########## modules/cli/src/main/java/org/apache/ignite/internal/cli/core/repl/completer/DynamicCompleterRegistry.java: ########## @@ -38,60 +40,97 @@ public class DynamicCompleterRegistry { public List<DynamicCompleter> findCompleters(String[] words) { return completionStrategiesList.stream() .filter(strategy -> strategy.canBeApplied(words)) - .map(CompletionStrategy::completer) + .map(strategy -> strategy.completer(words)) .collect(Collectors.toList()); } - public void register(DynamicCompleter completer) { - completionStrategiesList.add(new CompletionStrategy(ignored -> true, completer)); - } - /** Registers dynamic completer that can be found by given predicate. */ - public void register(Predicate<String[]> predicate, DynamicCompleter completer) { - completionStrategiesList.add(new CompletionStrategy(predicate, completer)); - } + public void register(CompleterConf conf, DynamicCompleterFactory factory) { + if (conf.isExclusiveEnableOptions()) { + // add disable option for all strategies because current configuration has exclusive enable option + completionStrategiesList.forEach(strategy -> strategy.exclusiveDisableOptions.addAll(conf.enableOptions())); + } - /** Registers dynamic completer that can be found by given prefix. */ - public void register(String[] prefixWords, DynamicCompleter completer) { - register((String[] words) -> samePrefix(words, prefixWords), completer); - } + Set<String> exclusiveDisableOptions = completionStrategiesList.stream() + .filter(strategy -> strategy.conf.isExclusiveEnableOptions()) + .flatMap(strategy -> strategy.conf.enableOptions().stream()) + .collect(Collectors.toSet()); - /** Registers dynamic completer that can be found by given prefix. */ - public void register(String[] prefixWords, String[] stopPostfixWords, DynamicCompleter completer) { - register((String[] words) -> samePrefix(words, prefixWords) && notSamePostfix(words, stopPostfixWords), completer); + CompletionStrategy strategy = new CompletionStrategy(conf, factory); + strategy.exclusiveDisableOptions.addAll(exclusiveDisableOptions); + completionStrategiesList.add(strategy); } - private boolean samePrefix(String[] words, String[] prefixWords) { - if (words.length < prefixWords.length) { - return false; + private static class CompletionStrategy { + private final CompleterConf conf; + + private final DynamicCompleterFactory factory; + + private final Set<String> exclusiveDisableOptions = new HashSet<>(); + + private CompletionStrategy(CompleterConf conf, DynamicCompleterFactory factory) { + this.conf = conf; + this.factory = factory; } - for (int i = 0; i < prefixWords.length; i++) { - if (!words[i].equals(prefixWords[i])) { + + private static boolean samePrefix(String[] words, String[] prefixWords) { + if (words.length < prefixWords.length) { return false; } + for (int i = 0; i < prefixWords.length; i++) { + if (!words[i].equals(prefixWords[i])) { + return false; + } + } + return true; } - return true; - } - - private boolean notSamePostfix(String[] words, String[] stopPostfixWords) { - return words.length > 0 && !Set.of(stopPostfixWords).contains(findLastNotEmptyWord(words)); - } - private static class CompletionStrategy { - private final Predicate<String[]> predicate; - private final DynamicCompleter completer; + boolean canBeApplied(String[] words) { + // empty command means can be applied to all words + if (!conf.commandSpecific()) { + return canBeAppliedCommandMatch(words); + } - private CompletionStrategy(Predicate<String[]> predicate, DynamicCompleter completer) { - this.predicate = predicate; - this.completer = completer; + Optional<String[]> commandsMatch = conf.commands().stream().filter(command -> samePrefix(words, command)).findFirst(); + return commandsMatch.isPresent() && canBeAppliedCommandMatch(words); } - boolean canBeApplied(String[] words) { - return predicate.test(words); + private boolean canBeAppliedCommandMatch(String[] words) { + String cursorWord = words[words.length - 1]; + String lastNotEmptyWord = findLastNotEmptyWord(words); + String preLastNotEmptyWord = findLastNotEmptyWordBeforeWordFromEnd(words, lastNotEmptyWord); + + if (cursorWord.equals(lastNotEmptyWord)) { + if (exclusiveDisableOptions.contains(lastNotEmptyWord) || exclusiveDisableOptions.contains(preLastNotEmptyWord)) { + return false; + } + } else if (exclusiveDisableOptions.contains(lastNotEmptyWord)) { + return false; + } + + if (conf.hasEnableOptions()) { + if (cursorWord.equals(lastNotEmptyWord)) { + return conf.enableOptions().contains(lastNotEmptyWord) // command subcommand --enable-option + || conf.enableOptions().contains(preLastNotEmptyWord); // command subcommand --enable-option lastWord + } else { + return conf.enableOptions().contains(lastNotEmptyWord); // command subcommand --enable-option <space> + } + } + + return !conf.hasDisableOptions() + + || (conf.hasDisableOptions() + && !cursorWord.equals(lastNotEmptyWord) + && !conf.disableOptions().contains(lastNotEmptyWord)) + + || (conf.hasDisableOptions() + && cursorWord.equals(lastNotEmptyWord) + && !conf.disableOptions().contains(lastNotEmptyWord) + && !conf.disableOptions().contains(preLastNotEmptyWord)); Review Comment: This expression has the same logic as the if statements above, so maybe we can replace one with the other so they are consistent? ########## modules/cli/src/test/java/org/apache/ignite/internal/cli/core/repl/completer/DynamicCompleterRegistryTest.java: ########## @@ -36,6 +40,11 @@ class DynamicCompleterRegistryTest { DynamicCompleter completer3; + /** Makes reading more easy. */ Review Comment: ```suggestion /** Makes reading easier. */ ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
