always set default js #280

[artlog]

No description provided.

[rouazana]

Thank you for this contribution!

I understand that this solution should work, but it seems a little too much for me to create an error implementation just to handle the case when the default implementation is null.

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.

On the other hand if you think that defaultImplementation should 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.

[rouazana]

If understand well, this change means if you don't find any "js" implementation, you defaults on "rjs" which is in fact Rhino.
It's hard to understand that when reading your code at first sight. Wouldn't it be possible to make it more obvious?

[artlog]

yes you understand well, trying to get the better default implementation.

it might be possible to make it more obvious, but making things obvious is not as obvious as it sound ;-)
Does just a comment would help ?
Or {"js","rjs"} set in a variable named 'preferredDefaultImplementations' ?

[rouazana]

I think I would just be more explicit, even if it means repeating myself:

defaultImplementation = instancesTypeCache.get("js");
if (defaultImplementation == null) {
  defaultImplementation = instancesTypeCache.get("rjs");
}

with the optional solution it's even easier to write it:

			defaultImplementation = Optional.ofNullable(instancesTypeCache.get("js"))
					.or(() -> Optional.ofNullable(instancesTypeCache.get("rjs")));

It's very easy and clear (to me at least ^^) how to add new cases if needed later (I guess it's the purpose of your array).

[artlog]

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 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 defaultImplementation should 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.

[rouazana]

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 defaultImplementation should 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.

[artlog]

see #304