From 1aea390b9bb3b575f7f28bc15c6e3a9f24bdfdaa Mon Sep 17 00:00:00 2001 From: Curtis Rueden Date: Sun, 19 Jun 2016 16:01:48 -0400 Subject: [PATCH 1/5] ScriptInfo: remove unthrown exception clause --- src/main/java/org/scijava/script/ScriptInfo.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/scijava/script/ScriptInfo.java b/src/main/java/org/scijava/script/ScriptInfo.java index 6d4657be6..73356e07e 100644 --- a/src/main/java/org/scijava/script/ScriptInfo.java +++ b/src/main/java/org/scijava/script/ScriptInfo.java @@ -391,7 +391,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 attrs = new HashMap<>(); attrs.put("type", "OUTPUT"); addItem(ScriptModule.RETURN_VALUE, Object.class, attrs); From 77e09424607aa2d3ed902ad13e389ea1326772d9 Mon Sep 17 00:00:00 2001 From: Curtis Rueden Date: Sun, 19 Jun 2016 16:11:38 -0400 Subject: [PATCH 2/5] Invert + generalize script return value logic There may be other reasons affecting the decision to append the script's return value as an extra output. Let's be flexible. --- .../java/org/scijava/script/ScriptInfo.java | 29 ++++++++++++++----- .../java/org/scijava/script/ScriptModule.java | 2 +- 2 files changed, 22 insertions(+), 9 deletions(-) diff --git a/src/main/java/org/scijava/script/ScriptInfo.java b/src/main/java/org/scijava/script/ScriptInfo.java index 73356e07e..c0c8a65d2 100644 --- a/src/main/java/org/scijava/script/ScriptInfo.java +++ b/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 -- @@ -372,7 +372,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. */ @@ -479,4 +484,12 @@ private static String getReaderContentsAsString(final Reader reader) return builder.toString(); } + // -- Deprecated methods -- + + /** @deprecated Use {@link #isReturnValueAppended()} instead. */ + @Deprecated + public boolean isReturnValueDeclared() { + return !isReturnValueAppended(); + } + } diff --git a/src/main/java/org/scijava/script/ScriptModule.java b/src/main/java/org/scijava/script/ScriptModule.java index 602545ebe..ae06eb9d7 100644 --- a/src/main/java/org/scijava/script/ScriptModule.java +++ b/src/main/java/org/scijava/script/ScriptModule.java @@ -190,7 +190,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; } From 230c267177babaa35e82794e787a8916bac565fc Mon Sep 17 00:00:00 2001 From: Curtis Rueden Date: Sun, 19 Jun 2016 16:18:50 -0400 Subject: [PATCH 3/5] ScriptModule: track return value explicitly Rather than relying on the return value being stored as an extra output, let's save the return value as its own reference. That way, in cases where the return value does _not_ get appended as an extra output, we still have a handle on it. --- src/main/java/org/scijava/script/ScriptModule.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/scijava/script/ScriptModule.java b/src/main/java/org/scijava/script/ScriptModule.java index ae06eb9d7..45d2028db 100644 --- a/src/main/java/org/scijava/script/ScriptModule.java +++ b/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)); From 2eb86fb5688b32a35dc1bc8483dc1393c1d493d7 Mon Sep 17 00:00:00 2001 From: Curtis Rueden Date: Sun, 19 Jun 2016 16:21:52 -0400 Subject: [PATCH 4/5] ScriptInfo: append return value if no outputs We want the return value to be an easy way to return a single, untyped (Object) output. This is quite useful for the "lazy" (I mean "elegant") script writer. But for those consuming scripts, it is annoying to always have to deal with this extra "result" output. As a compromise, let's only append the "result" output if: A) no other outputs were declared; and B) "result" was not declared as an input either. --- src/main/java/org/scijava/script/ScriptInfo.java | 7 ++++++- src/test/java/org/scijava/script/ScriptInfoTest.java | 8 ++------ 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/main/java/org/scijava/script/ScriptInfo.java b/src/main/java/org/scijava/script/ScriptInfo.java index c0c8a65d2..efb1b26cf 100644 --- a/src/main/java/org/scijava/script/ScriptInfo.java +++ b/src/main/java/org/scijava/script/ScriptInfo.java @@ -412,7 +412,12 @@ private void addItem(final String name, final Class 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 void assignAttribute(final DefaultMutableModuleItem item, diff --git a/src/test/java/org/scijava/script/ScriptInfoTest.java b/src/test/java/org/scijava/script/ScriptInfoTest.java index 53f9ed659..6073a8d1a 100644 --- a/src/test/java/org/scijava/script/ScriptInfoTest.java +++ b/src/test/java/org/scijava/script/ScriptInfoTest.java @@ -97,7 +97,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,10 +169,6 @@ 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()) { @@ -180,7 +176,7 @@ public void testParameters() { } int outputCount = 0; - final ModuleItem[] outputs = { buffer, result }; + final ModuleItem[] outputs = { buffer }; for (final ModuleItem outItem : info.outputs()) { assertSame(outputs[outputCount++], outItem); } From 1e1020437a9d9a014de23d906f7ed9bbcaf2bb18 Mon Sep 17 00:00:00 2001 From: Curtis Rueden Date: Sun, 19 Jun 2016 16:30:15 -0400 Subject: [PATCH 5/5] Test the improved script return value behavior When _no_ explicit @OUTPUT is given, the return value _is_ appended. When an explicit @OUTPUT _is_ given, the return value is _not_ appended. --- .../org/scijava/script/ScriptInfoTest.java | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/src/test/java/org/scijava/script/ScriptInfoTest.java b/src/test/java/org/scijava/script/ScriptInfoTest.java index 6073a8d1a..37b85214c 100644 --- a/src/test/java/org/scijava/script/ScriptInfoTest.java +++ b/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 is 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 outputs = scriptModule.getOutputs(); + assertEquals(1, outputs.size()); + assertTrue(outputs.containsKey(ScriptModule.RETURN_VALUE)); + } + + /** + * Tests that the return value is not 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 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.