SHL-170: Allow space-text for values

Author: ericbottard

.gitignore

@@ -13,3 +13,4 @@ target/
 /*.iml
 /*.ipr
 /*.iws
+out

src/main/java/org/springframework/shell/core/SimpleParser.java

@@ -216,22 +216,14 @@ public ParseResult parse(final String rawInput) {
 
 				// Ensure the user specified a value if the value is mandatory or
 				// key and value must appear in pair
-				boolean mandatory = !StringUtils.hasText(value) && cliOption.mandatory();
-				boolean specifiedKey = !StringUtils.hasText(value) && options.containsKey(sourcedFrom);
-				boolean specifiedKeyWithoutValue = false;
-				if (specifiedKey) {
-					value = cliOption.specifiedDefaultValue();
-					if ("__NULL__".equals(value)) {
-						specifiedKeyWithoutValue = true;
-					}
-				}
-				if (mandatory || specifiedKeyWithoutValue) {
-					if ("".equals(cliOption.key()[0])) {
-						StringBuilder message = new StringBuilder("You should specify a default option ");
-						if (cliOption.key().length > 1) {
-							message.append("(otherwise known as option '").append(cliOption.key()[1]).append("') ");
-						}
-						message.append("for this command");
+				boolean mandatory = cliOption.mandatory();
+				boolean specifiedKey = options.containsKey(sourcedFrom);
+				boolean specifiedKeyWithoutValue = specifiedKey && "".equals(value);
+				if (mandatory && (!specifiedKey || specifiedKeyWithoutValue)) {
+					if (isDefaultOption(cliOption)) {
+						StringBuilder message = new StringBuilder("You should specify ");
+						message.append(optionAliases(cliOption));
+						message.append(" for this command");
 						LOGGER.warning(message.toString());
 					}
 					else {
@@ -241,12 +233,10 @@ public ParseResult parse(final String rawInput) {
 				}
 
 				// Accept a default if the user specified the option, but didn't provide a value
-				if ("".equals(value)) {
+				if (specifiedKeyWithoutValue) {
 					value = cliOption.specifiedDefaultValue();
-				}
-
+				} else if (!specifiedKey) {
 				// Accept a default if the user didn't specify the option at all
-				if (value == null) {
 					value = cliOption.unspecifiedDefaultValue();
 				}
 
@@ -336,6 +326,42 @@ private void resetCompletionInvocations() {
 		successiveCompletionRequests = 1;
 	}
 
+	/**
+	 * Return true if the given option is the 'default' option, that is, one of its key is the empty String.
+	 */
+	private boolean isDefaultOption(CliOption option) {
+		for (String key : option.key()) {
+			if ("".equals(key)) {
+				return true;
+			}
+		}
+		return false;
+	}
+
+	/**
+	 * Return a plain string description of the available aliases for a given option.
+	 */
+	private CharSequence optionAliases(CliOption option) {
+		StringBuilder result = new StringBuilder();
+		if (isDefaultOption(option)) {
+			result.append("a default option");
+		}
+		if (option.key().length > 1) {
+			result.append(", otherwise known as ");
+		}
+		boolean comma = false;
+		for (String key : option.key()) {
+			if (!"".equals(key)) {
+				if (comma) {
+					result.append(", ");
+				}
+				result.append("'").append(key).append("'");
+				comma = true;
+			}
+		}
+		return result;
+	}
+
 	private void reportTokenizingException(String commandKey, TokenizingException te) {
 		StringBuilder caret = new StringBuilder();
 		for (int i = 0; i < te.getOffendingOffset() + commandKey.length() + 1; i++) {
@@ -364,7 +390,7 @@ private void printHintMessage(final Set<CliOption> cliOptions, Map<String, Strin
 			boolean found = false;
 			for (String key : keys) {
 				if (options.containsKey(key)) {
-					if (!StringUtils.hasText(options.get(key))) {
+					if (!StringUtils.hasLength(options.get(key))) {
 						valueBuilder.append(key);
 						valueBuilder.append("' for this command");
 						hintForOptions = false;

src/test/java/org/springframework/shell/core/SimpleParserTests.java

@@ -16,15 +16,7 @@
 
 package org.springframework.shell.core;
 
-import static org.hamcrest.Matchers.containsString;
-import static org.hamcrest.Matchers.empty;
-import static org.hamcrest.Matchers.endsWith;
-import static org.hamcrest.Matchers.equalTo;
-import static org.hamcrest.Matchers.hasItem;
-import static org.hamcrest.Matchers.is;
-import static org.hamcrest.Matchers.not;
-import static org.hamcrest.Matchers.nullValue;
-import static org.hamcrest.Matchers.startsWith;
+import static org.hamcrest.Matchers.*;
 import static org.junit.Assert.assertThat;
 
 import java.util.ArrayList;
@@ -58,6 +50,28 @@ public class SimpleParserTests {
 
 	private ArrayList<Completion> candidates = new ArrayList<Completion>();
 
+	@Test
+	public void testSHL170_ValueMadeOfSpacesOK() {
+		parser.add(new MyCommands());
+		parser.add(new StringConverter());
+
+		// A value made of space(s) is ok
+		ParseResult result = parser.parse("bar --option1 ' '");
+		assertThat(result.getMethod().getName(), equalTo("bar"));
+		assertThat(result.getArguments(), arrayContaining((Object) " "));
+
+		// Ok too with a literal TAB
+		result = parser.parse("bar --option1 '\t'");
+		assertThat(result.getMethod().getName(), equalTo("bar"));
+		assertThat(result.getArguments(), arrayContaining((Object)"\t"));
+
+		// Also Ok when Shell itself does the escaping
+		result = parser.parse("bar --option1 '\\t'");
+		assertThat(result.getMethod().getName(), equalTo("bar"));
+		assertThat(result.getArguments(), arrayContaining((Object)"\t"));
+
+	}
+
 	@Test
 	public void testSimpleCommandNameCompletion() {
 		parser.add(new MyCommands());