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

Added an example of the ScriptAction preset #2228

Merged
merged 7 commits into from
May 30, 2024

Conversation

OlegAndreych
Copy link
Contributor

No description provided.

Copy link

netlify bot commented Feb 7, 2024

Thanks for your pull request to the openHAB documentation! The result can be previewed at the URL below (this comment and the preview will be updated if you add more commits).

Name Link
🔨 Latest commit 721ae9e
🔍 Latest deploy log https://app.netlify.com/sites/openhab-docs-preview/deploys/65c3da1afdb84d0009470e51
😎 Deploy Preview https://deploy-preview-2228--openhab-docs-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Feb 7, 2024

Thanks for your pull request to the openHAB documentation! The result can be previewed at the URL below (this comment and the preview will be updated if you add more commits).

Name Link
🔨 Latest commit 212d74d
🔍 Latest deploy log https://app.netlify.com/sites/openhab-docs-preview/deploys/664ba0ac488d8e00080acea3
😎 Deploy Preview https://deploy-preview-2228--openhab-docs-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@rkoshak
Copy link
Contributor

rkoshak commented Feb 7, 2024

You should mention that this is not necessary when using JS Scripting nor jRuby as all of that stuff is handled by the helper libraries.

Nashorn JS will work similarly to Groovy apparently.

JS Scripting

actions.ScriptExecution.createTimer('MyTimer', time.toZDT(5000), () => { console.info('Timer ran') });

jRuby

after(5.seconds) { logger.info('Timer ran' }

Nashorn JS

scriptExtension.importPreset("ScriptAction")
var ZonedDateTime = Java.type('java.time.ZonedDateTime');
var logger = org.slf4j.LoggerFactory.getLogger('Test logger');
scriptExecution.createTimer(ZonedDateTime.now.plusSeconds(5), function() {
  logger.info('Timer ran');
});

But this raises a question. Should everything provided by ScriptExecution be documented here? Id does more than just create timers (i.e. callScript) and there are several actions for creating timers.

And what about all the other built in actions found here? If ScriptExecution is going to be separately documented then all of them should be. But the content will get pretty redundant I think. Maybe the section should be made generic and show how to import all of these with a couple of examples.

I also know that for at least JS Scripting, the classes are just imported directly instead of using the rule presents. I don't know if there is a big difference or preference.

e.g.
https://github.com/openhab/openhab-js/blob/308cca8ec0a616b190cf2b90d1a8fe3a05b25d30/src/actions.js#L196

@OlegAndreych
Copy link
Contributor Author

OlegAndreych commented Feb 8, 2024

I don’t see anything besides timers neither on the org.openhab.core.automation.module.script.action.ScriptExecution interface nor on the org.openhab.core.automation.module.script.internal.action.ScriptExecutionImpl implementation.

Moreover, in the org.openhab.core.automation.module.script.internal.action.ScriptActionScriptScopeProvider scriptExecution is defined as org.openhab.core.automation.module.script.action.ScriptExecution. That’s the way the context variable appears in scripts.

That’s why only timers are documented.

I also know that for at least JS Scripting, the classes are just imported directly instead of using the rule presents. I don’t know if there is a big difference or preference.

As far as I can see (by running test UI script), there’s no scriptExecution context variable in Groovy scripts without the scriptExtension.importPreset(“ScriptAction”).

The doc page we're talking about states that “The default preset is preloaded, so it does not require importing.”. No other presets are mentioned, I don’t see ‘em imported on practice, and I don’t see anything from these presets is available without importing these presets.

@rkoshak
Copy link
Contributor

rkoshak commented Feb 8, 2024

I don’t see anything besides timers neither on the org.openhab.core.automation.module.script.action.ScriptExecution interface nor on the org.openhab.core.automation.module.script.internal.action.ScriptExecutionImpl implementation.

callScript is on ScriptExecution too. https://www.openhab.org/javadoc/latest/org/openhab/core/model/script/actions/scriptexecution

The doc page we're talking about states that “The default preset is preloaded, so it does not require importing.”. No other presets are mentioned, I don’t see ‘em imported on practice, and I don’t see anything from these presets is available without importing these presets.

It might be that Groovy works differently from the other languages. If so, the documentation for stuff that Groovy does differently belongs in the docs for Groovy. I don't know.

But anything that goes in the JSR223 Docs needs to be generic and inclusive. You will notice that all the other examples on this page includes most of the supported languages, not just one. It's worth noting that Groovy is indeed missing but that's because all the people who have edited this doc until now don't know Groovy. It's not one of the most used languages and no one has really volunteered over the years to provide good documentation for it.

If you do know Groovy, it would be most helpful to add Groovy examples to the other sections. Though that's probably best left for a different PR.

@OlegAndreych
Copy link
Contributor Author

OlegAndreych commented Feb 8, 2024

callScript is on ScriptExecution too.

Fair enough. But no other built-in actions you've mentioned earlier.

I'd prefer that someone else who has used callScript in practice would volunteer a doc for it.

Even without describing an interface in all its entirety, I'll consider doc with current additions more complete, hence in a better state than before.
There always will be things to add and refine.

It might be that Groovy works differently from the other languages. If so, the documentation for stuff that Groovy does differently belongs in the docs for Groovy. I don't know.

We have a part of the doc about importing presets. It is not marked as specific for any language and not under tab:

ScriptExtension Objects (all JSR223 languages)

To facilitate JSR223 scripting, several openHAB-related variables are automatically predefined within ScriptExtension presets. They can be loaded into the script context using scriptExtension.importPreset(String preset), e.g. scriptExtension.importPreset("RuleSimple"). The default preset is preloaded, so it does not require importing.

If I understand you correctly, you're saying that this is Groovy-specific functionality but, it's in the doc already.

It's worth noting that Groovy is indeed missing but that's because all the people who have edited this doc until now don't know Groovy.

But anything that goes in the JSR223 Docs needs to be generic and inclusive.

I see this doc with its raw class names and internal details as an overview of the JSR233 scripting mechanism from the openHAB standpoint as a platform, where language specific examples provided as an addition for convenience.

I can remove the part where I mention that "preset can be useful for scheduling asynchronous code execution with org.openhab.core.automation.module.script.action.Timer". This part indeed can be considered Groovy-specific. An example can be removed too.

If you do know Groovy, it would be most helpful to add Groovy examples to the other sections. Though that's probably best left for a different PR.

I'll consider it.

@rkoshak
Copy link
Contributor

rkoshak commented Feb 8, 2024

Fair enough. But no other built-in actions you've mentioned earlier.

Right. Those are on different presets. But if we are going to document ScriptExecution, shouldn't we also document HTTP, Exec, Ephemeris, Log, Semantics, and all the rest of the built in Actions? All of these:

https://www.openhab.org/javadoc/latest/org/openhab/core/model/script/actions/package-summary

There isn't anything special about ScriptExecution. It's just one of many built in rules Actions.

And because it would be all quite redundant to document each one individually, I'm proposing making the update here more generic to all of these instead of iterating over each and every one with 90% of the same docs and only the names of the presets/classes/examples changing. Provide the names of the presets and one or two examples and point to https://www.openhab.org/docs/configuration/actions.html for the full list of Actions and how to use them.

If I understand you correctly, you're saying that this is Groovy-specific functionality but, it's in the doc already.

No, what I'm saying is that the need to import it manually like you've documented is Groovy specific. All the rest of the languages get it by default, as the documents currently say should happen.

I can remove the part where I mention that "preset can be useful for scheduling asynchronous code execution with org.openhab.core.automation.module.script.action.Timer". This part indeed can be considered Groovy-specific. An example can be removed too.

That part too is true for all the languages. What's Groovy specific is you don't just have access to the presets to call these by default like is the case in the other languages (except Nashorn though I'd need to explore more there too as it should be there in the default presets but I've never seen it pulled in that way, it's usually imported using Java.type()).

@OlegAndreych
Copy link
Contributor Author

shouldn't we also document HTTP, Exec, Ephemeris, Log, Semantics, and all the rest of the built in Actions?

There isn't anything special about ScriptExecution. It's just one of many built in rules Actions.

Do these actions have their own presets? I can't find them.

This PR is not about rules Actions. It's about one particular preset and one particular variable this preset brings into the script context. This preset exists as a part of a JSR223 scripting mechanism of the openHAB platform and isn't special for any scripting language. If some of the scripting language's implementations don't require explicit preset import, so what? It is still there in generic JSR223 code, which this doc is about considering its title.

As one can see, in some cache examples there is cache preset import, and some don't have it. Is it considered as an issue?

No, what I'm saying is that the need to import it manually like you've documented is Groovy specific.

That part is under the Groovy tab of examples. Isn't something placed under a language-specific tab considered language-specific? Isn't that why these tabs are there in the first place?

If you're saying that there should be tabs for other languages, then sure. I'll be happy to use examples you provided in your first comment. If that wouldn't be enough, then I'd prefer to get more precise examples of what you want to be in the doc, or leave that part for someone who's proficient in scripting using these languages for the openHAB platform.

It's rather hard for me to get your point about what this doc is intended to look like.

@stefan-hoehn
Copy link
Contributor

@rkoshak Let me know and ping me when you finally feel comfortable to have it merged.

@rkoshak
Copy link
Contributor

rkoshak commented Feb 12, 2024

Do these actions have their own presets? I can't find them.

I would assume so but I don't know where to look. I can't imagine why ScriptExecution would have a preset and the rest wouldn't. There is nothing special about it.

Is it considered as an issue?

It's only an issue if the only example presented is Groovy. I supplied examples for many of the other languages above. Rules DSL gets what it gets so I'm not sure an example for it is worth anything.

That part is under the Groovy tab of examples. Isn't something placed under a language-specific tab considered language-specific? Isn't that why these tabs are there in the first place?

The tabs are to show how to do it in all the supported languages in a way that it doesn't take up pages of space showing each one. If this is something that is only done in Groovy, it should go in the Groovy docs. If it's something universal, there should be examples for most of the languages.

@OlegAndreych
Copy link
Contributor Author

I would assume so but I don't know where to look.

As far as I understand, context variables for JSR223 are provided by org.openhab.core.automation.module.script.ScriptExtensionProvider interface implementors. That's irrespective to the scripting language because JSR223 is a spec for a generic scripting mechanism.

Looking at org.openhab.core.automation.module.script.ScriptExtensionProvider class hierarchy I can see a org.openhab.core.automation.module.script.internal.action.ScriptActionScriptScopeProvider that provides scriptExecution context variable we're discussing. But I don't see any ScriptExtensionProvider for any other Action.

There's a default context variable actions provided by org.openhab.core.automation.module.script.internal.defaultscope.DefaultScriptScopeProvider, but that's really different because it provides an untyped a generic access mechanism to thing actions.

So now you know where to look and can do it for yourself.

It's only an issue if the only example presented is Groovy.

In my previous comment, I've suggested to add your examples to the relevant tabs:

If you're saying that there should be tabs for other languages, then sure. I'll be happy to use examples you provided in your first comment.

Would that be enough?

@rkoshak
Copy link
Contributor

rkoshak commented Feb 14, 2024

OK, I wrote a rule to dump all the presets available from the ScriptExtensionProvider.

This is Nashorn JS:

var logger = Java.type('org.openhab.core.model.script.actions.Log');

function dumpPreset(presetName) {
  logger.logInfo('test', 'Dumping Preset  ' + presetName);
  var preset = scriptExtension.importPreset(presetName);
  var keys = preset.keySet();
  logger.logInfo('test', '  Preset Variables: ' + keys);
  keys.forEach(function(key) {
    logger.logInfo('test', '   ' + key);
    var obj = preset.get(key);
    logger.logInfo('test', '    ' + Java.typeName(obj.getClass()));    
  });
}

var presets = scriptExtension.getPresets();
presets.forEach(function(presetName) { dumpPreset(presetName); });
Preset Documented Provides Purpose
lifecycle No lifecycleTracker I think this lets one register a function to be called when the rule is disposed. See LifecycleScriptExtensionProvider.LifecycleTracker, maybe not actually useful for end users inside rules?
ScriptAction Subject of this PR scriptExecution See ScriptExecution maybe? It's referring to an "internal" class in the preset dump I cannot find in the JavaDocs.
cache Yes
RuleSimple Yes
RuleSupport Yes
RuleFactories Yes
default Yes
media No voice and audio See VoiceManager and AudioManager
osgi No Access to OSGI services This one gets complicated.

We are missing quite a bit more than just the ScriptAction preset which, I still wonder why it even exists given that none of the other Rule Actions are exposed as a preset and there shouldn't be anything special about creating a Timer that warrants a wholly separate preset.

Regardless I'd recommend one of these two options.

  1. Add the documentation for ScriptAction, following the overall approach as shown in the cache section. Then create Issues for each of lifecycle, media, and osgi.
  2. Add documentation for these other missing presets to this PR.

I can help with 2. I've never used them except for osgi but now that I know what's there I can experiment to figure it out.

Copy link
Contributor

@stefan-hoehn stefan-hoehn left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution but I wonder if @rkoshak is happy now?

@stefan-hoehn stefan-hoehn added this to the 4.2 milestone May 19, 2024
@rkoshak
Copy link
Contributor

rkoshak commented May 20, 2024

I'm happy with what's been written except for the fact that Rules DSL, JS Scripting and jRuby are missing. Beyond that, fi it's not covered in this PR, an issue needs to be created to add sections for lifecycle, media and osgi.

The other examples would be:

:::: tabs

::: tab Groovy

scriptExtension.importPreset("ScriptAction")

scriptExecution.createTimer(ZonedDateTime.now().plusSeconds(1), () -> {
  org.slf4j.LoggerFactory.getLogger('Test logger').warn('Timer ran')
})

:::

::: tab JSScripting

actions.scriptExecution.createTimer(time.toZDT(1000), () => {
  console.info('Timer ran');
}
:::

::: tab DSL

createTimer(now.plusSeconds(1), [ |
  logInfo('Test logger', 'Timer ran')
])

:::

::: tab JRuby

my_timer = after(1.seconds) do
  logger.info('Timer ran');
end
:::

@stefan-hoehn
Copy link
Contributor

Do you like me to add these to the PR?

@rkoshak
Copy link
Contributor

rkoshak commented May 20, 2024

That would be great! I was trying to figure out how to add them myself and I'm not sure how to go about doing it.

@jimtng
Copy link
Contributor

jimtng commented May 20, 2024

The Ruby version:

after(1.second) do
  logger.info("Timer ran")
end

@stefan-hoehn
Copy link
Contributor

stefan-hoehn commented May 20, 2024

@rkoshak no worries, that's what I am here for. 👋 As a maintainer I can add to the PR of the contributor (and there is an easy way to do that by using 'gh pr checkout 2228'. Also since a few weeks you can even run "npm run serve" to build the docs locally to verify the outcome before pushing.

@jimtng
The difference I see to Rich's proposal is that your is 1.second whereas Rich proposed 1.seconds ? Also you omitted the variable assignment - was that intended?

Note: I had to fix a few markdown issues as well.

Please Rich and Jim check my changes.

Signed-off-by: Oleg Andreych <kjiec4@gmail.com>
Signed-off-by: Oleg Andreych <kjiec4@gmail.com>
@jimtng
Copy link
Contributor

jimtng commented May 20, 2024

The difference I see to Rich's proposal is that your is 1.second whereas Rich proposed 1.seconds ? Also you omitted the variable assignment - was that intended?

Note: I had to fix a few markdown issues as well.

Please Rich and Jim check my changes.

Please use my version as is

  • No semicolons
  • Double quotes instead of single quotes

@stefan-hoehn
Copy link
Contributor

stefan-hoehn commented May 20, 2024

Unfortunately the rebase broke a few things. I need to clean that up first 😱
And I had to fix some other markdown issues as well.

Let me know if I can merge it.

Signed-off-by: Stefan Höhn <mail@stefanhoehn.com>
Signed-off-by: Stefan Höhn <mail@stefanhoehn.com>
Signed-off-by: Stefan Höhn <mail@stefanhoehn.com>
Signed-off-by: Stefan Höhn <mail@stefanhoehn.com>

```ruby
after(1.second) do
logger.info("Timer ran")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
logger.info("Timer ran")
logger.info("Timer ran")

Signed-off-by: Stefan Höhn <mail@stefanhoehn.com>
@stefan-hoehn stefan-hoehn merged commit 2cd2741 into openhab:main May 30, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants