Merge pull request #242 from scijava/return-values

Improve behavior of return value outputs

Author: ctrueden

src/main/java/org/scijava/script/ScriptInfo.java

@@ -94,8 +94,8 @@ public class ScriptInfo extends AbstractModuleInfo implements Contextual {
 	@Parameter
 	private ConvertService convertService;
 
-	/** True iff the return value is explicitly declared as an output. */
-	private boolean returnValueDeclared;
+	/** True iff the return value should be appended as an output. */
+	private boolean appendReturnValue;
 
 	/**
 	 * Creates a script metadata object which describes the given script file.
@@ -229,7 +229,7 @@ public BufferedReader getReader() {
 	@Override
 	public void parseParameters() {
 		clearParameters();
-		returnValueDeclared = false;
+		appendReturnValue = true;
 
 		try {
 			final BufferedReader in;
@@ -254,7 +254,7 @@ public void parseParameters() {
 			}
 			in.close();
 
-			if (!returnValueDeclared) addReturnValue();
+			if (appendReturnValue) addReturnValue();
 		}
 		catch (final IOException exc) {
 			log.error("Error reading script: " + path, exc);
@@ -264,9 +264,9 @@ public void parseParameters() {
 		}
 	}
 
-	/** Gets whether the return value is explicitly declared as an output. */
-	public boolean isReturnValueDeclared() {
-		return returnValueDeclared;
+	/** Gets whether the return value is appended as an additional output. */
+	public boolean isReturnValueAppended() {
+		return appendReturnValue;
 	}
 
 	// -- ModuleInfo methods --
@@ -374,7 +374,12 @@ private void parseParam(final String param,
 		}
 		final Class<?> type = scriptService.lookupClass(typeName);
 		addItem(varName, type, attrs);
-		if (ScriptModule.RETURN_VALUE.equals(varName)) returnValueDeclared = true;
+
+		if (ScriptModule.RETURN_VALUE.equals(varName)) {
+			// NB: The return value variable is declared as an explicit OUTPUT.
+			// So we should not append the return value as an extra output.
+			appendReturnValue = false;
+		}
 	}
 
 	/** Parses a comma-delimited list of {@code key=value} pairs into a map. */
@@ -393,7 +398,7 @@ private void checkValid(final boolean valid, final String param)
 	}
 
 	/** Adds an output for the value returned by the script itself. */
-	private void addReturnValue() throws ScriptException {
+	private void addReturnValue() {
 		final HashMap<String, Object> attrs = new HashMap<>();
 		attrs.put("type", "OUTPUT");
 		addItem(ScriptModule.RETURN_VALUE, Object.class, attrs);
@@ -409,7 +414,12 @@ private <T> void addItem(final String name, final Class<T> type,
 			assignAttribute(item, key, value);
 		}
 		if (item.isInput()) registerInput(item);
-		if (item.isOutput()) registerOutput(item);
+		if (item.isOutput()) {
+			registerOutput(item);
+			// NB: Only append the return value as an extra
+			// output when no explicit outputs are declared.
+			appendReturnValue = false;
+		}
 	}
 
 	private <T> void assignAttribute(final DefaultMutableModuleItem<T> item,
@@ -481,4 +491,12 @@ private static String getReaderContentsAsString(final Reader reader)
 		return builder.toString();
 	}
 
+	// -- Deprecated methods --
+
+	/** @deprecated Use {@link #isReturnValueAppended()} instead. */
+	@Deprecated
+	public boolean isReturnValueDeclared() {
+		return !isReturnValueAppended();
+	}
+
 }

src/main/java/org/scijava/script/ScriptModule.java

@@ -88,6 +88,8 @@ public class ScriptModule extends AbstractModule implements Contextual {
 	/** Destination for standard error during script execution. */
 	private Writer error;
 
+	private Object returnValue;
+
 	public ScriptModule(final ScriptInfo info) {
 		this.info = info;
 	}
@@ -130,7 +132,7 @@ public ScriptEngine getEngine() {
 
 	/** Gets the return value of the script. */
 	public Object getReturnValue() {
-		return getOutput(RETURN_VALUE);
+		return returnValue;
 	}
 
 	// -- Module methods --
@@ -167,7 +169,7 @@ public void run() {
 		}
 
 		// execute script!
-		Object returnValue = null;
+		returnValue = null;
 		try {
 			final Reader reader = getInfo().getReader();
 			if (reader == null) returnValue = engine.eval(new FileReader(path));
@@ -190,7 +192,7 @@ public void run() {
 			final String name = item.getName();
 			if (isResolved(name)) continue;
 			final Object value;
-			if (RETURN_VALUE.equals(name) && !getInfo().isReturnValueDeclared()) {
+			if (RETURN_VALUE.equals(name) && getInfo().isReturnValueAppended()) {
 				// NB: This is the special implicit return value output!
 				value = returnValue;
 			}

src/test/java/org/scijava/script/ScriptInfoTest.java

@@ -32,6 +32,7 @@
 package org.scijava.script;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertSame;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
@@ -45,6 +46,7 @@
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
 
 import javax.script.Bindings;
 import javax.script.ScriptContext;
@@ -84,6 +86,42 @@ public static void tearDown() {
 
 	// -- Tests --
 
+	/**
+	 * Tests that the return value <em>is</em> appended as an extra output when no
+	 * explicit outputs were declared.
+	 */
+	@Test
+	public void testReturnValueAppended() throws Exception {
+		final String script = "" + //
+			"% @LogService log\n" + //
+			"% @int value\n";
+		final ScriptModule scriptModule =
+			scriptService.run("include-return-value.bsizes", script, true).get();
+
+		final Map<String, Object> outputs = scriptModule.getOutputs();
+		assertEquals(1, outputs.size());
+		assertTrue(outputs.containsKey(ScriptModule.RETURN_VALUE));
+	}
+
+	/**
+	 * Tests that the return value is <em>not</em> appended as an extra output
+	 * when explicit outputs were declared.
+	 */
+	@Test
+	public void testReturnValueExcluded() throws Exception {
+		final String script = "" + //
+			"% @LogService log\n" + //
+			"% @OUTPUT int value\n";
+		final ScriptModule scriptModule =
+			scriptService.run("exclude-return-value.bsizes", script, true).get();
+
+		final Map<String, Object> outputs = scriptModule.getOutputs();
+		assertEquals(1, outputs.size());
+		assertTrue(outputs.containsKey("value"));
+		assertFalse(outputs.containsKey(ScriptModule.RETURN_VALUE));
+	}
+
+
 	/**
 	 * Ensures parameters are parsed correctly from scripts, even in the presence
 	 * of noise like e-mail addresses.
@@ -97,7 +135,7 @@ public void testNoisyParameters() throws Exception {
 		final ScriptModule scriptModule =
 			scriptService.run("hello.bsizes", script, true).get();
 
-		final Object output = scriptModule.getOutput("result");
+		final Object output = scriptModule.getReturnValue();
 
 		if (output == null) fail("null result");
 		else if (!(output instanceof Integer)) {
@@ -169,18 +207,14 @@ public void testParameters() {
 		assertItem("buffer", StringBuilder.class, null, ItemIO.BOTH, true, true,
 			null, null, null, null, null, null, null, null, noChoices, buffer);
 
-		final ModuleItem<?> result = info.getOutput("result");
-		assertItem("result", Object.class, null, ItemIO.OUTPUT, true, true, null,
-			null, null, null, null, null, null, null, noChoices, result);
-
 		int inputCount = 0;
 		final ModuleItem<?>[] inputs = { log, sliderValue, animal, buffer };
 		for (final ModuleItem<?> inItem : info.inputs()) {
 			assertSame(inputs[inputCount++], inItem);
 		}
 
 		int outputCount = 0;
-		final ModuleItem<?>[] outputs = { buffer, result };
+		final ModuleItem<?>[] outputs = { buffer };
 		for (final ModuleItem<?> outItem : info.outputs()) {
 			assertSame(outputs[outputCount++], outItem);
 		}