Allow rule with no trigger in DSL Rule file format#5462
Conversation
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
|
This is requested by #3655. If there are no triggers, then |
There was a problem hiding this comment.
Pull request overview
This PR updates the openHAB DSL rules grammar to allow rule declarations whose when section contains no triggers, and adds integration tests to validate that such rules are loaded with an empty trigger list.
Changes:
- Make the
EventTriggerlist in thewhenclause optional inRules.xtext. - Extend existing rule-loading assertions to check trigger/action list sizes.
- Add an integration test covering a rule with no triggers (
whenimmediately followed bythen).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| itests/org.openhab.core.model.rule.tests/src/main/java/org/openhab/core/model/rule/runtime/DSLRuleProviderTest.java | Adds coverage for “no trigger” DSL rules and strengthens assertions for trigger/action list sizes. |
| bundles/org.openhab.core.model.rule/src/org/openhab/core/model/rule/Rules.xtext | Updates Xtext grammar so when can contain zero triggers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… parameter It was already called with StandardCharsets.UTF_8 for certain tests in that test class. Signed-off-by: Laurent Garnier <lg.hc@free.fr>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Reading the discussion in #3655 , I am now not sure this change is expected, @kaikreuzer was against it, arguing that a rule with no trigger is in fact a DSL Script. And this is not wrong :) My intention was just to allow the conversion of such UI rules in DSL Rule file format. |
|
But without this change, there will be DSL UI rules that cannot be converted to DSL rules, right? That seems like a good enough reason to support it. If not, maybe the export to DSL file format could generate a .script, but there needs to be some way to tell the user that's what happened. And if one selects multiple rules to export the scripts need to be filtered out so they are not all put in the same file with the rules. |
Yes. |
|
In the current discussion, I do not see why a trigger, which can never trigger, allows writing a rule, but a rule without a trigger should be illegal. An example for a trigger, which never triggers, is to wait for an ONOFF/Switch item to change its state to OPEN. If a Such rules have no practical usages, but DSL Rules/Scripts/Xbase is a programming language and programming languages allow programmers to write things, which have no practical usages. It is all in the risk zone of the one, who writes the rules. |
My main goal is to have consistent concepts towards our users. Back in the discussion in #3655, a "block of logic" that can be executed (manually on the console, called from within a rule or as the execution block of a rule) was a "Script". The concept of Scripts does not only exist in DSL, but is also embraced by the UI - Scripts are here described as "Rules dedicated to running code". I would hence claim that rules without triggers are either not yet completely defined or they should be listed as Scripts in the UI - and with that there would be no issue in transforming them to DSL Scripts. If we say that the concept of Rules is generally also meant to merely define reusable blocks of logic and that triggers are not an important part of a rule, we should imho think about deprecating the concept of Scripts. I'm fine with either way. |
I would actually be in favor of this because the term "Script" has too many meanings in OH already. But that points to a limitation with your comparison between Rules DSL Scripts and MainUI Scipts. Today the term "Script" referrs to all of these separate concepts:
It is possible to create a rule in the UI without triggers now without making them a Script. And there are some good reasons to want to do so. One example I personally use is I have handlers which are called by rule templates. It is a lot easier if those handlers are there next to the rule that calls them rather than a completely different place in the UI (which is where they would be if they were made as a UI script). But another case is I'm not always ready to add the triggers yet because the rule is under development. It would be a problem if I created a rule in the UI without a trigger and the UI automatically moved it to Scripts. First of all, UI Scripts only support Script Actions. There are all sorts of other Actions possible in UI rules. Any Rule that uses these cannot become a Script. Secondly, it's possible for a rule without triggers to have a condition. UI Scripts cannot have a condition. Finally, consider the following scenario:
Rules can be complicated things requiring multiple editing sessions to create fully. It would have a negative impact on usability if we made that impossible. Obviously, all of these restrictions and work flow can change, but the above is what would happen as of today if we rejected rules without triggers and/or automatically moved them to be scripts. |
I absolutely agree, that was definitely not what I suggested. Most/all of your examples fall into the category that I called "not yet completely defined" rules, which are not scripts and must not appear/moved to Scripts. Clearly, there is no visual or technical differentiation between a "completely defined" and "not completely defined" rule in openHAB and I am not sure whether it would make sense (as it would possibly require to introduce a new rule status like "incomplete", which again complicates things and needs to be explained to the users).
I'd be interested to know the current usage of "Scripts" (both in UI and DSL), which I cannot really judge. If we'd go and deprecate them, how many users would be annoyed because they are building on it (and like the distinction)? I personally could live with it, but I have no clue about others... |
|
If deprecated Rules DSL Scripts and added Another/additional approach could be to figure out how to make a Rules DSL script that behaves more like what we would call a library in other languages, particularly adding the ability for scripts to accept arguments. Then we could use "callLibraryFunction" or something like that and treat DSL scripts like libraries instead of calling another rule. Of course,
I think we would need to decide, is For quite some time now, Rules DSL Scripts have been somewhat of an outlier in how they are written, how they are invoked, and what they can do when compared to all the other rules languages. If consistency is our goal, we need additional work to make DSL Scripts behave more like the other languages, either through a |
|
In the current changes, when a rule has no triggers, I suggest to allow skipping the What will mean from users’ perspective that DSL Scripts are deprecated? For me this means removing the distinction between DSL Rules and DSL Scripts. Allowing in DSL Rules to write after the In other words, in the same way there is no distinction between DSL Rules and DSL Scripts should be understood, as if they were an add-on, which can be substituted with the Groovy, Nashorn, Jython, etc automation add-on. |
I'm for deprecating the concept of Scripts too, because it's a "fragile construct" that is full of challenges. I looked at the UI code, and this is solved simply by filtering rules. For all intents and purposes, they are rules. This is reflected in the REST API, and when you e.g. want to export these to YAML. As more possibilities are added around rules, it gets harder and harder to "maintain the illusion". That doesn't mean that Scripts must be "deprecated". The UI menu option (which just applies a predefined filter to rules) can stay as long as it's seen useful. They wouldn't disappear or stop working in any way, but they would show up together with other rules as well. Which would be consistent with what can be observed through the REST API, YAML, the JSON DB etc. If we are to try to preserve the concept of "Scripts" as a different entity than rules, I think my only option when making the conversion code is to block conversion of "Scripts". Alternatively, a completely new structure to handle these must be designed and implemented. |
|
Whatever is decided, my work with converting rules is blocked until a decision is made, because I must know what to do with these "objects". |
|
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/rules-and-rule-templates-yaml-integration/168568/206 |
|
@kaikreuzer : what is the final decision? Depending on the decision, either I close that PR or I need to resolve the conflicts. |
|
@lolodomo I cannot take a final decision here. I'd suggest to have @openhab/core-maintainers agree on the solution. I personally am ok with this change, but as discussed, I would then also suggest to retire/deprecate the concept of scripts. Might reduce the overall complexity. |
When you say this, I assume you mean to remove the special "view" in the UI that shows rules with Does it include stopping support for the Does it also include removing the Personally I would leave In my mind, enabling DSL to call other rules doesn't mean that you have to remove what's there. One option would be to remove it from the documentation (and document the "new way" of doing it instead), but still leave the functionality intact. That still prevents breaking stuff. Or, is "the confusion" of having this old "script functionality" enough of a burden that it justifies breaking installations that happen to use it? |
|
In my view, Rules DSL should be aligned as much as possible with the other languages. The scripts concept for Rules DSL has always been different then what we call scripts for other languages and what you see under scripts in the UI. Under the above two, I am all for deprecating the specific DSL script support. In my view, that does not mean remove it immediately, but remove it from the documentation and put in some logging when it is used, discouraging its usage. Would it be an option to parse DSL scripts into rules without trigger? DSL |
As far as I can tell, yes. There is also the challenge of receiving arguments in DSL (like triggering event, sent parameters etc.), but I dabbled with that and found ways it could be achieved as well, and since it's not possible with DSL scripts today either, it doesn't really affect "parity" here.
I haven't looked at the details, but can't quite understand that it should be hard to achieve. I think they are already treated internally as just that, a rule without a trigger (that automatically gets the "Script" tag attached), so this "change" would probably mostly be cosmetic. I'm not sure how UID collisions are avoided though. DSL UIDs are created from the filename, which must be unique within the same folder, but this isn't the case since we're dealing with two different folders
|
Yes, this means: after the |
I certainly don't follow your reasoning here. I read it as "aligned from a user perspective", not attempted forced to behave like other scripting languages internally. I'm not saying that all the things you suggest are "bad", but I think each one of them would have to make sense on their own. I really think the whole idea of separating DSL out as an add-on is bad, it doesn't only handle "scripting", it also provides file formats. Making it an add-on would mean that |
|
I have received this message over email:
When I visit Moving the bundles However this was not what I suggested. I proposed moving the bundles |
|
Externalizing everything that uses xtext in separate addons is certainly not what I meant. This is another, and much bigger, debate. It has been debated before, and it is not on the table right now. |
|
And off-topic in the context of my PR. |
|
So back to the topic:
This was my idea here, yes. Not breaking compatibility, but discouraging users to use it and rather document the "no-trigger rules" as the way to go instead. I do not see anybody arguing against this PR here, so @lolodomo, I would say that you might want to rebase it! |
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
lolodomo
force-pushed
the
dsl_rule_no_trigger
branch
from
ee16e99 to
d2d3b3b
Compare
|
@lolodomo I assume there is some documentation to be updated. |
No description provided.