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

[automation] Pass script context to script engines #1837

Merged
merged 1 commit into from Nov 22, 2020

Conversation

jpg0
Copy link
Contributor

@jpg0 jpg0 commented Nov 19, 2020

Pass explicit script context to script engines. Context initially includes only engine identifier.

Signed-off-by: Jonathan Gilbert jpg@trillica.com

@jpg0 jpg0 changed the title Pass script context to script engines [automation] Pass script context to script engines Nov 19, 2020

SimpleScriptContext scriptContext = new SimpleScriptContext();
scriptContext.setAttribute(CONTEXT_KEY_ENGINE_IDENTIFIER, engineIdentifier, ScriptContext.ENGINE_SCOPE);
engine.setContext(scriptContext);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it safe to set that here? Couldn't the script engine have already set up some initial context?
I'm especially concerned about the DSLScriptEngine, where I recently had a lot of trouble getting the handling of the context stable... Would be good if you could test that there's no issue by this on DSL rules.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kaikreuzer Good point; I was primarily thinking about the newer script engines which I don't believe set things up here. TBH my main goal here is passing context to script engine factories upon creation of script engines to allow them to be tailored.

The cleanest way to do this would be to modify the engineFactory.createScriptEngine(scriptTypes.get(0)); call eariler in the method to add a custom ScriptEngineContext object as a second argument. However I chose not to do this because it would be a breaking change for existing implementors of the interface declaring this method (of course I could also introduce a 2nd interface and collect implementations of both, but that seems overkill). Or do you think this would be a better option? If not, I'll update the code to add context to whatever is there already (and create if not present) before testing on the DSLScriptEngine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Updated code to only set context if it's not already been set)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the current solution is fine, thanks.

@kaikreuzer
Copy link
Member

Did you notice that the build fails due to spotless errors?

Signed-off-by: Jonathan Gilbert <jpg@trillica.com>
@jpg0
Copy link
Contributor Author

jpg0 commented Nov 21, 2020

I had not, thanks for pointing it out. Build is now passing.

Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

@kaikreuzer kaikreuzer merged commit 60edebc into openhab:master Nov 22, 2020
@kaikreuzer kaikreuzer added this to the 3.0.0.M3 milestone Nov 22, 2020
@kaikreuzer kaikreuzer added automation enhancement An enhancement or new feature of the Core labels Nov 22, 2020
@jpg0 jpg0 deleted the scriptengine-context branch November 24, 2020 03:54
splatch pushed a commit to ConnectorIO/copybara-hab-core that referenced this pull request Jul 11, 2023
Signed-off-by: Jonathan Gilbert <jpg@trillica.com>
GitOrigin-RevId: 60edebc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automation enhancement An enhancement or new feature of the Core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants