ScriptEngineFactory.createScriptEngine() - describe purpose of parameter

[dilyanpalauzov]

I reached this conclusion after looking at all .createScriptEngine() calls in openhab-core.

[kaikreuzer]

@dilyanpalauzov Please make sure to sign your commits to make the DCO check pass, see https://github.com/openhab/openhab-core/blob/main/CONTRIBUTING.md#sign-your-work.

[kaikreuzer]

Pascal and PHP

Let me guess: This text is AI slop? It would be wonderful if you could review AI generated texts yourself before creating PRs from them...

In general, I do not see a value of the changes suggested in this PR - I think the current description is crisp and precise. Don't you think?

[dilyanpalauzov]

The text is written by me.

I think the return type of createScriptEngine should be changed to @NonNull.

The purpose of the change is that the implementations of ScriptEngineFactory can rely on the fact, that they are called in createScriptEngine with an argument, which the ScriptEngineFactory has returned by getScriptTypes().

So in the createScriptEngine(String scriptType) implementations there should be no need to check, if the scriptType value is valid.

Signing with DCO here does not solve any problems. In general assigning explicitly copyright is only meaningful in situations, where one can describe financial losses towards somebody. The current change cannot be a justification for financial losses (from whom towards whom?).

[kaikreuzer]

The text is written by me.

Ok, sorry for assuming otherwise.
Why did you choose Pascal and PHP as examples here? These are definitely no languages for which ScriptEngines exist.

I think the return type of createScriptEngine should be changed to @nonnull.

This does not seem to be what this PR suggests - at least you kept it as @Nullable.

can rely on the fact, that they are called in createScriptEngine with an argument, which the ScriptEngineFactory has returned

This is the definition of an interface here. How could you rely on anything that implementations do?

Signing with DCO here does not solve any problems.

It does. It makes sure to respect the project's contribution guidelines without requiring any off-topic discussions.

[dilyanpalauzov]

I selected PHP and Pascal to show that a single ScriptEngineFactory can implement (offer) any number of completely different, random languages. And to know which language is meant, the parameter of createScriptEngine() exists.

Annotation @NonNull is not something that this proposal did, but I can change it.

Yes, part of the definitions of the interface is to write down as text what purpose the parameters serve. And my finding for this parameter is that it serves a purpose only when the ScriptEngineFactory implements many different languages (e.g JavaScript 1, and the completely different language JavaScript 3). So writing down how it works is a way to narrow down the interface in a backward-compatible manner.

How can I rely on what OH-JSR223 implementations do? openHAB is the only thing which initializes (calls functions) from these implementations, so OH can write down (as comment) how it intends to use the parameter.

Currently there is no advice when createScriptEngine() is supposed to return null and when instead it should throw an exception.

[kaikreuzer]

I selected PHP and Pascal to show that a single ScriptEngineFactory can implement (offer) any number of completely different, random languages

Ok, I nonetheless find it a bit misleading as readers might assume that there are implementations in openHAB for these languages. Why not choose Python and Ruby as an example?

Annotation @nonnull is not something that this proposal did, but I can change it.

Thinking about it, I would actually say that it should be @NonNull and throw an exception in case of an error - something like an IllegalArgumentException for example. This still gives the implementation the chance to react to a parameter that it cannot handle, while indicating to the caller of the method that in general a script engine instance is expected back. Wdyt?

[dilyanpalauzov]

The point of writing this text down is to make clear, what cannot be expressed by the Java type system: automation add-ons (JSR223 implementations) are assured that the passed argument to createScriptEngine() is one of the strings returned by getScriptTypes(). In turn the code in the addons, checking if the passed argument is one returened previously by getScriptTypes(), can be deleted. In other words, if Java had allowed in a easy way to return by getScriptTypes() a set of enums and createScriptEngine() is forced to accept one value of that set, then this comment would not have been necessary.

There is no IllegalArgumentException involved - the JSR223 implementations can assume that the argument is valid, as clarified by the current text, so they do not have to check if it is valid.

Currently it is not clear when should an automation add-on return null on createScriptEngine() as the argument is always valid. It would make sense if the list of supported languages/mime types by an JSR223 add-on changes dynamically over time, but this is nowhere the case currently and is overengineering.

As this discussion moves slowly, I will rephrase the text and remove this change

-    * @return ScriptEngine or null
+    * @return ScriptEngine or, in case of error, null

so as to leave unspecified when null can be returned and leave out the @NonNull annotation. This will make the changeset smaller and thus will accelerate its integration. Later a distinct proposal for adding @Nonnull and removing or null above can be made.

I nonetheless find it a bit misleading as readers might assume that there are implementations in openHAB for these languages. Why not choose Python and Ruby as an example?

This would also be misleading, as then users migth assume that there is a single add-on which offers Ruby and Python at the same time. I will remove the mentioning of any languages as examples.

[kaikreuzer]

Thanks.
Merging with small patch exception rule.