Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ScriptEngine API Fixes #231

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

ScriptEngine API Fixes #231

wants to merge 4 commits into from

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

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.

None yet

3 participants