From 3b1c3e273dab37f72c81c87d70b5697d48af9f54 Mon Sep 17 00:00:00 2001 From: Eric Bottard Date: Thu, 6 Feb 2014 13:39:09 +0100 Subject: [PATCH] Add tests and fix completion corner cases --- gradle.properties | 2 +- .../shell/core/SimpleParser.java | 47 ++-- .../springframework/shell/core/Tokenizer.java | 42 +++- .../shell/core/SimpleParserTests.java | 204 ++++++++++++++++++ .../shell/core/TokenizerTests.java | 4 +- 5 files changed, 262 insertions(+), 37 deletions(-) create mode 100644 src/test/java/org/springframework/shell/core/SimpleParserTests.java diff --git a/gradle.properties b/gradle.properties index b8decade..153a8e35 100644 --- a/gradle.properties +++ b/gradle.properties @@ -11,7 +11,7 @@ jlineVersion=2.11 # Testing -junitVersion = 4.10 +junitVersion = 4.11 mockitoVersion = 1.8.5 hamcrestDateVersion = 0.9.3 diff --git a/src/main/java/org/springframework/shell/core/SimpleParser.java b/src/main/java/org/springframework/shell/core/SimpleParser.java index 35be09ae..23a74e4f 100644 --- a/src/main/java/org/springframework/shell/core/SimpleParser.java +++ b/src/main/java/org/springframework/shell/core/SimpleParser.java @@ -545,7 +545,7 @@ public class SimpleParser implements Parser { if (targets.isEmpty()) { // Nothing matches the buffer they've presented - return cursor; + return -1; } if (targets.size() > 1) { // Assist them locate a particular target @@ -658,7 +658,7 @@ public class SimpleParser implements Parser { // Handle if they are trying to find out the available option keys; always present option keys in order // of their declaration on the method signature, thus we can stop when mandatory options are filled in - if (methodTarget.getRemainingBuffer().endsWith("--") && !tokenizer.lastValueHadQuote()) { + if (methodTarget.getRemainingBuffer().endsWith("--") && !tokenizer.lastValueIsStillBeingTyped()) { boolean showAllRemaining = true; for (CliOption include : unspecified) { if (include.mandatory()) { @@ -685,7 +685,7 @@ public class SimpleParser implements Parser { // option key/value pair) if (lastOptionKey == null || (!"".equals(lastOptionKey) && !"".equals(lastOptionValue) && translated.endsWith(" ") && !tokenizer - .lastValueHadQuote())) { + .lastValueIsStillBeingTyped())) { // We have either NEVER specified an option key/value pair // OR we have specified a full option key/value pair @@ -749,11 +749,9 @@ public class SimpleParser implements Parser { } // Handle completing the option key they're presently typing - if ((lastOptionValue == null || "".equals(lastOptionValue)) - && !(translated.endsWith(" ") || translated.endsWith(" \""))) { - // Given we haven't got an option value of any form, and there's no space at the buffer end, we must - // still be typing an option key - // System.out.println("completing an option"); + if ("".equals(lastOptionValue)) { + // Given we haven't got an option value of any form, we must + // still be typing an option key. for (CliOption option : cliOptions) { for (String value : option.key()) { if (value != null && lastOptionKey != null @@ -821,21 +819,12 @@ public class SimpleParser implements Parser { } } - String prefix = ""; - if (!tokenizer.lastValueHadQuote() && !translated.endsWith(" ")) { - prefix = " "; - } - else if (tokenizer.lastValueHadQuote()) { - // Re-install opening quote if there was one - prefix = " \""; - } - // Only include in the candidates those results which are compatible with the present buffer for (Completion currentValue : allValues) { // We only provide a suggestion if the lastOptionValue == "" if (!StringUtils.hasText(lastOptionValue)) { // We should add the result, as they haven't typed anything yet - results.add(new Completion(prefix + currentValue.getValue() + suffix, currentValue + results.add(new Completion(currentValue.getValue() + suffix, currentValue .getFormattedValue(), currentValue.getHeading(), currentValue.getOrder())); } else { @@ -843,10 +832,12 @@ public class SimpleParser implements Parser { // already typed it in full if (currentValue.getValue().toLowerCase().startsWith(lastOptionValue.toLowerCase()) && !lastOptionValue.equalsIgnoreCase(currentValue.getValue()) - && lastOptionValue.length() < currentValue.getValue().length()) { - results.add(new Completion(prefix + currentValue.getValue() + suffix, - currentValue.getFormattedValue(), currentValue.getHeading(), - currentValue.getOrder())); + && lastOptionValue.length() < currentValue.getValue().length() + && (tokenizer.getLastValueDelimiter() == ' ' || tokenizer + .lastValueIsStillBeingTyped())) { + results.add(new Completion(currentValue.getValue() + suffix, currentValue + .getFormattedValue(), currentValue.getHeading(), currentValue + .getOrder())); } } } @@ -889,18 +880,14 @@ public class SimpleParser implements Parser { if (suggestion.equals(lastOptionValue)) { // They have pressed TAB in the default value, and the default value has already // been provided as an explicit option - return 0; + return -1; } } if (results.size() > 0) { candidates.addAll(results); - if (tokenizer.lastValueHadQuote()) { - return translated.lastIndexOf(" \""); - } - else { - return translated.lastIndexOf(" ") + 1; - } + return methodTarget.getKey().length() + " ".length() + + tokenizer.getLastValueStartOffset(); } return 0; } @@ -908,7 +895,7 @@ public class SimpleParser implements Parser { } } - return 0; + return -1; } } diff --git a/src/main/java/org/springframework/shell/core/Tokenizer.java b/src/main/java/org/springframework/shell/core/Tokenizer.java index 8fcd4cd3..e9d66919 100644 --- a/src/main/java/org/springframework/shell/core/Tokenizer.java +++ b/src/main/java/org/springframework/shell/core/Tokenizer.java @@ -54,7 +54,11 @@ public class Tokenizer { /** * Used to indicate that the last value was indeed half enclosed in quotes. Useful so that parser can re-add it. */ - private boolean lastValueHadQuote; + private boolean lastValueIsStillBeingTyped; + + private char lastValueDelimiter; + + private int lastValueStartOffset; public Tokenizer(String text) { this(text, false); @@ -114,6 +118,9 @@ public class Tokenizer { endDelimiter = '"'; pos++; } + // So that it can be retrieved later (if this is actually the last value) + lastValueDelimiter = endDelimiter; + lastValueStartOffset = pos; while (pos < buffer.length && buffer[pos] != endDelimiter) { if (buffer[pos] == ESCAPE_CHAR && pos + 1 < buffer.length && buffer[pos + 1] == endDelimiter) { sb.append(endDelimiter); @@ -130,7 +137,7 @@ public class Tokenizer { (buffer[pos - 1] != '"' || // quotes are not properly closed sb.length() == 0)) { // BUT it's ok if consumed nothing (pos-1 is *opening* quote then) if (allowUnbalancedLastQuotedValue) { - lastValueHadQuote = true; + lastValueIsStillBeingTyped = true; return sb.toString(); } else { @@ -142,8 +149,25 @@ public class Tokenizer { return sb.toString(); } - public boolean lastValueHadQuote() { - return lastValueHadQuote; + /** + * Return the offset at which the last value seen started (NOT including any delimiter). + */ + public int getLastValueStartOffset() { + return lastValueStartOffset; + } + + /** + * Return the delimiter (space or quotes) that was (or is being) used for the last value. + */ + public char getLastValueDelimiter() { + return lastValueDelimiter; + } + + /** + * Return whether the last value was meant to be enclosed in quotes, but the closing quote has not been typed yet. + */ + public boolean lastValueIsStillBeingTyped() { + return lastValueIsStillBeingTyped; } /** @@ -173,4 +197,14 @@ public class Tokenizer { } return new String(buffer, start, pos - start); } + + @Override + public String toString() { + StringBuilder result = new StringBuilder().append(buffer).append('\n'); + for (int i = 0; i < lastValueStartOffset; i++) { + result.append(' '); + } + result.append('^'); + return result.toString(); + } } diff --git a/src/test/java/org/springframework/shell/core/SimpleParserTests.java b/src/test/java/org/springframework/shell/core/SimpleParserTests.java new file mode 100644 index 00000000..30ffd381 --- /dev/null +++ b/src/test/java/org/springframework/shell/core/SimpleParserTests.java @@ -0,0 +1,204 @@ +/* + * Copyright 2013 the original author or authors. + * + * Licensed 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.springframework.shell.core; + +import static org.hamcrest.Matchers.empty; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.hasItem; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.startsWith; +import static org.junit.Assert.assertThat; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; + +import org.hamcrest.Description; +import org.hamcrest.DiagnosingMatcher; +import org.hamcrest.Matcher; +import org.junit.Test; +import org.springframework.shell.core.annotation.CliCommand; +import org.springframework.shell.core.annotation.CliOption; + +/** + * Tests for parsing and completion logic. + * + * @author Eric Bottard + */ +public class SimpleParserTests { + + private SimpleParser parser = new SimpleParser(); + + private int offset; + + private String buffer; + + private ArrayList candidates = new ArrayList(); + + @Test + public void testSimpleCommandNameCompletion() { + parser.add(new MyCommands()); + + buffer = "f"; + offset = parser.completeAdvanced(buffer, buffer.length(), candidates); + + assertThat(candidates, hasItem(completionThat(is(equalTo("foo"))))); + } + + @Test + public void testSimpleArgumentNameCompletion() { + parser.add(new MyCommands()); + + buffer = "bar --op"; + offset = parser.completeAdvanced(buffer, buffer.length(), candidates); + + assertThat(candidates, hasItem(completionThat(is(equalTo("bar --option1 "))))); + } + + @Test + public void testSimpleArgumentValueCompletion() { + parser.add(new MyCommands()); + parser.add(new StringCompletions(Arrays.asList("abc", "def", "ghi"))); + + buffer = "bar --option1 a"; + offset = parser.completeAdvanced(buffer, buffer.length(), candidates); + + assertThat(candidates, hasItem(completionThat(is(equalTo("bar --option1 abc"))))); + } + + @Test + public void testArgumentValueCompletionWhenQuoted() { + parser.add(new MyCommands()); + parser.add(new StringCompletions(Arrays.asList("abc", "def", "ghi"))); + + buffer = "bar --option1 \"a"; + offset = parser.completeAdvanced(buffer, buffer.length(), candidates); + + assertThat(candidates, hasItem(completionThat(is(equalTo("bar --option1 \"abc"))))); + } + + @Test + public void testCompletionInMiddleOfBuffer() { + parser.add(new MyCommands()); + + buffer = "bar --optimum"; + offset = parser.completeAdvanced(buffer, "bar --opti".length(), candidates); + + assertThat(candidates, hasItem(completionThat(is(equalTo("bar --option1 "))))); + + } + + @Test + public void testArgumentValueCompletionWhenAmbiguity() { + parser.add(new MyCommands()); + parser.add(new StringCompletions(Arrays.asList("abc", "def", "abd"))); + + buffer = "bar --option1 a"; + offset = parser.completeAdvanced(buffer, buffer.length(), candidates); + + assertThat(candidates, hasItem(completionThat(startsWith("bar --option1 ab")))); + } + + @Test + public void testArgumentValueCompletionWhenAmbiguityUsingQuotes() { + parser.add(new MyCommands()); + parser.add(new StringCompletions(Arrays.asList("abc", "def", "abd"))); + + buffer = "bar --option1 \"a"; + offset = parser.completeAdvanced(buffer, buffer.length(), candidates); + + assertThat(candidates, hasItem(completionThat(is(equalTo("bar --option1 \"abc"))))); + assertThat(candidates, hasItem(completionThat(is(equalTo("bar --option1 \"abd"))))); + } + + @Test + public void testArgumentValueCompletionUnderstandsEndQuote() { + parser.add(new MyCommands()); + parser.add(new StringCompletions(Arrays.asList("abc", "def", "abd"))); + + buffer = "bar --option1 \"ab\""; + offset = parser.completeAdvanced(buffer, buffer.length(), candidates); + + assertThat(candidates, is(empty())); + } + + /** + * Return a matcher that asserts that a completion, when added to {@link #buffer} at the given {@link #offset}, + * indeed matches the provided matcher. + */ + private Matcher completionThat(final Matcher matcher) { + return new DiagnosingMatcher() { + + public void describeTo(Description description) { + description.appendText("a completion that ").appendDescriptionOf(matcher); + } + + @Override + protected boolean matches(Object item, Description mismatchDescription) { + Completion completion = (Completion) item; + StringBuilder sb = new StringBuilder(buffer); + sb.setLength(offset); + sb.append(completion.getValue()); + boolean match = matcher.matches(sb.toString()); + mismatchDescription.appendText("result was ") + .appendValue(sb.insert(offset, '[').append(']').toString()); + return match; + } + }; + } + + public static class MyCommands implements CommandMarker { + + @CliCommand("foo") + public void foo() { + + } + + @CliCommand("bar") + public void bar(@CliOption(key = "option1") + String option1) { + + } + } + + public static class StringCompletions implements Converter { + + private final List completions; + + public StringCompletions(List completions) { + this.completions = completions; + } + + public boolean supports(Class type, String optionContext) { + return type == String.class; + } + + public String convertFromText(String value, Class targetType, String optionContext) { + return value; + } + + public boolean getAllPossibleValues(List completions, Class targetType, String existingData, + String optionContext, MethodTarget target) { + for (String s : this.completions) { + completions.add(new Completion(s)); + } + return false; + } + + } + +} diff --git a/src/test/java/org/springframework/shell/core/TokenizerTests.java b/src/test/java/org/springframework/shell/core/TokenizerTests.java index ff369262..67355a4a 100644 --- a/src/test/java/org/springframework/shell/core/TokenizerTests.java +++ b/src/test/java/org/springframework/shell/core/TokenizerTests.java @@ -132,7 +132,7 @@ public class TokenizerTests { expected.put("foo", "bar bazz"); expected.put("bizz", "unfinished bizness "); assertEquals(expected, result); - assertTrue(tokenizer.lastValueHadQuote()); + assertTrue(tokenizer.lastValueIsStillBeingTyped()); } @Test @@ -143,7 +143,7 @@ public class TokenizerTests { expected.put("foo", "bar bazz"); expected.put("bizz", ""); assertEquals(expected, result); - assertTrue(tokenizer.lastValueHadQuote()); + assertTrue(tokenizer.lastValueIsStillBeingTyped()); } @Test