Skip to content

ScriptEngine API Fixes#231

Open
akbertram wants to merge 4 commits intomasterfrom
script-engine-fixes
Open

ScriptEngine API Fixes#231
akbertram wants to merge 4 commits intomasterfrom
script-engine-fixes

Conversation

@akbertram
Copy link
Member

Changes to Renjin's ScriptEngine implementation to bring it closer to correctness with the API documentation and with the reference Javascript implementation.

Assert.assertNull(bindingsFromContext.get(key));

bindingsFromEngine.put(key, value);
Assert.assertEquals(bindingsFromEngine.get(key), value);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@peter-gergely-horvath does the API require that value from get(key) is equal to the original value set?

Renjin's implementation automatically converts scalars like instances of java.lang.String or `java.lang.Number' to an R object (org.renjin.sexp.SEXP), so what you put in does not always equal what you get out.

Copy link
Contributor

@Alain-Bearez Alain-Bearez Nov 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the Javadoc:

Retrieves a value set in the state of this engine. The value might be one which was set using setValue or some other value in the state of the ScriptEngine, depending on the implementation.

https://docs.oracle.com/javase/8/docs/api/javax/script/ScriptEngine.html#get-java.lang.String-

This makes sense if you consider as example an implementation for the JavaScript language: while you put a string value for a key in your bindings, the execution of some script might result in the variable to get an array value.

As a consequence, the assertions on lines 165 and 167 have to be rewritten considering the fact that Renjin wraps the given java.lang.String value into an expression of type StringArrayVector.

    Assert.assertEquals(StringVector.valueOf(value), bindingsFromEngine.get(key));

    Assert.assertEquals(StringVector.valueOf(value), bindingsFromContext.get(key));

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants