-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add Rules Concepts page #3
Conversation
This covers 1 and 1a of the table at openhab#1855. Signed-off-by: Florian Hotze <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an excellent start. Thank you for getting it going!
Except for the minor grammar and formatting comments the rest are thoughts that came to mind as I was reading. Feel free to ignore or argue back.
I'm assuming that there is more to be written here. But if not, I think we need a section for each of Trigger, Action, and Condition with at least a paragraph going down a little bit more into them. What sort of events are there in OH? What sorts of things can one do with an Action (at a high level, mainly interacting with Items and rule actions to cause devices to do something). What sort of conditions can we define (time, Item states, etc.).
Even though we might not have much to say, I think it's important to have the three terms appear as separate sections in the TOC at the top of the page. Repetition, repetition, repetition. I want the user to see those three terms over and over again.
rules/concepts.md
Outdated
| `Condition` | But Only If | The *but only if __c__* part: Which condition has to be met that the rule really runs? | | ||
|
||
The __a__, that causes the rule to run, is an so-called event in openHAB. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm torn here. We keep flip flopping between "event" and "trigger". I think we either need to stick to one or the other almost everywhere (I'd choose "trigger") or somewhere in this section make it very clear that:
- event = something that happened that is detectable by openHAB: time, system event, Item states and commands
- trigger = identifies an event that will cause a rule to run
The difference is subtle but I fear there might be some confusion if we are not absolutely clear and disciplined in our use of the terms. Events are happing all the time. The trigger is like a filter to identify just those events that we care about to trigger a given rule: a specific time, Item changed to ON, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will use trigger in that section and add a note about the event later in the triggers section.
Signed-off-by: Florian Hotze <[email protected]>
@rkoshak I will extend this PR with the sections for triggers, conditions, actions and so on. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay. All my comments are pretty much grammar and word choice type suggestions. The only big change is to move one sentence.
It looks good. I think all the content is there and we'll cover the rest in the other pages.
@rkoshak |
No worries. I'm on a business trip right now so can't do a whole lot to review this week anyway. |
Signed-off-by: Florian Hotze <[email protected]>
Okay, no problem. I finally found some time to address your review, all review comments with a 👍 reaction should be resolved from my point of view. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One spelling error and a suggestion for the table. After that I think it's ready to go. If you want to leave the table as is, that's fine too. It's just that given that the last column has two parts it kind of points to splitting them into two columns.
Signed-off-by: Florian Hotze <[email protected]>
I addressed your last review, should be ready for merging. How/Where to proceed with the triggers (1b of your issue) and so on? |
Perfect. Let's roll with it. Thanks for your work on this and putting up with my nit picking. My trips are over at least until Oct. so hopefully I can work more on this and we can start to make more progress. Just let me know what you are working on so we don't collide. I'll ask you to review what I do before I merge it too. I'm not sure how that works but we can figure that out. |
You are welcome! Currently I am on vacation, but when I am back home I am planning to continue work on the docs. Could you introduce one more column to your table at the issue where you can mark what has been completed yet? Maybe with a ✅? I'm curious to see how mutual reviewing works :) |
Looks like the dandy checklist TODO here can't be combined with a table so we'll just have to manage them individually. Note incase it isn't clear in the original issue, I intend all of 1 to be on the same page to reduce the number of entries on the left hand menu in the docs. The fact that we are going up to 8 pages total is already causing me some concerns. |
I‘ve seen your todo column at the issue, might be better using the ✅ emoji.
I thought that we would do it that way, but explanation is always good.
That‘s much, you are right, but as we write about many things I am okay with that . |
Referring to openhab#1855.
@rkoshak WDYT?
To know what parts of the docs this WIP PR tries to cover at the moment, just have a look at my commit messages.
I decided to write this under the rules section, but we can decide to move it to the getting started at any time.