lsc-project
always set default js #280
Hi @rouazana ,
Thanks for your review.
If you really think that defaultImplementation can be null in some cases, I think replacing it by an Optional<ScriptableEvaluator> would do the job. If the optional is empty, failing fast at this point is the good solution IMO.
I didn't though about Optional usage when i wrote this change. I agree this would allow to obtain a NoSuchElementException instead of NullPointer, what is better. Now i am wondering if replacing it with Optional would require more change at other places like using isPresent() and replacing direct call on implementation method to get() on it. Or do i miss something here ?
On the other hand if you think that
defaultImplementationshould never be null it would be better to ensure it's well initialized in every cases, and also to fail fast if it's not the case.
Yes i think it should never be null but it should not stop on exception if never used. Protecting against NullPointerException with a right error when used but not stop at initialisation if never used, what we can't reasonably know at initialisation time.
Script evaluation can occur in very simple case that don't seems to be related to scripting at first sight sometimes even for constants, delegating it to an implementation is there to display error at usage and not at initialisation. In the long run we could even provide a very simple default implementation instead of this MissingError that can handle very simple cases.
It was a catch all implementation, if ever default implementation was not used, failing at first initialisation might be too much.
By example if another scripting language is added that can't fit into default implementation, if all script usages specify explicitly a implementation then it is working smoothly even if no js implementation is found.
Hello,
Sorry for the late answer.
If you really think that defaultImplementation can be null in some cases, I think replacing it by an Optional would do the job. If the optional is empty, failing fast at this point is the good solution IMO.
I didn't though about Optional usage when i wrote this change. I agree this would allow to obtain a NoSuchElementException instead of NullPointer, what is better. Now i am wondering if replacing it with Optional would require more change at other places like using isPresent() and replacing direct call on implementation method to get() on it. Or do i miss something here ?
You can keep the Optional as an internal implementation details and never expose it. I've attached an example. 292-optional-example.patch.txt
With this method you'll keep the right exception you want while not changing the API and not having to implement a dummy class.
On the other hand if you think that
defaultImplementationshould never be null it would be better to ensure it's well initialized in every cases, and also to fail fast if it's not the case.Yes i think it should never be null but it should not stop on exception if never used. Protecting against NullPointerException with a right error when used but not stop at initialisation if never used, what we can't reasonably know at initialisation time.
Good point, thank you for the explanation.
I think everything in this issue has been done in #304.
See especially this commit: 418acd89c8b637630c20765d358aba71489441b7
For the record, the logic for default JS engine selection is: try GraalJS, else JS (Jscript Evaluator, Nashorn, GraalJS depending on Java version), else RhinoJS
Closing
