-
Notifications
You must be signed in to change notification settings - Fork 82
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
base: master
Are you sure you want to change the base?
Conversation
…getBindings() are consistent
Assert.assertNull(bindingsFromContext.get(key)); | ||
|
||
bindingsFromEngine.put(key, value); | ||
Assert.assertEquals(bindingsFromEngine.get(key), value); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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));
ScriptEngine API fixes - final touch
ef9d981
to
efef981
Compare
Changes to Renjin's ScriptEngine implementation to bring it closer to correctness with the API documentation and with the reference Javascript implementation.