openhab / openhab-docs

This repository contains the documentation for openHAB.
https://www.openhab.org/docs/
Other
271 stars 689 forks source link

Added an example of the `ScriptAction` preset #2228

Closed OlegAndreych closed 4 months ago

netlify[bot] commented 7 months ago

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 721ae9e4459c2b5f929acba556661d8033daa9ff
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.

netlify[bot] commented 7 months ago

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 212d74d7d9c4e64c8ed90b320c439e970f927e1d
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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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

rkoshak commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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.

rkoshak commented 4 months ago

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 commented 4 months ago

Do you like me to add these to the PR?

rkoshak commented 4 months ago

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 commented 4 months ago

The Ruby version:

after(1.second) do
  logger.info("Timer ran")
end
stefan-hoehn commented 4 months ago

@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.

jimtng commented 4 months ago

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

stefan-hoehn commented 4 months ago

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.