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

Add Rules Concepts page #3

Merged
merged 4 commits into from
Aug 8, 2022
Merged

Conversation

florian-h05
Copy link

@florian-h05 florian-h05 commented Jul 12, 2022

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.

This covers 1 and 1a of the table at
openhab#1855.

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Copy link
Owner

@rkoshak rkoshak left a 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 Show resolved Hide resolved
rules/concepts.md Outdated Show resolved Hide resolved
rules/concepts.md Outdated Show resolved Hide resolved
rules/concepts.md Outdated Show resolved Hide resolved
rules/concepts.md Outdated Show resolved Hide resolved
| `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.
Copy link
Owner

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.

Copy link
Author

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.

rules/concepts.md Outdated Show resolved Hide resolved
rules/concepts.md Outdated Show resolved Hide resolved
rules/concepts.md Outdated Show resolved Hide resolved
rules/concepts.md Outdated Show resolved Hide resolved
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
@florian-h05
Copy link
Author

@rkoshak
Thanks for your extensive feedback, please have a look at my changes.

I will extend this PR with the sections for triggers, conditions, actions and so on.

Copy link
Owner

@rkoshak rkoshak left a 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.

rules/concepts.md Outdated Show resolved Hide resolved
rules/concepts.md Outdated Show resolved Hide resolved
rules/concepts.md Outdated Show resolved Hide resolved
rules/concepts.md Outdated Show resolved Hide resolved
rules/concepts.md Outdated Show resolved Hide resolved
rules/concepts.md Outdated Show resolved Hide resolved
rules/concepts.md Outdated Show resolved Hide resolved
rules/concepts.md Outdated Show resolved Hide resolved
rules/concepts.md Outdated Show resolved Hide resolved
rules/concepts.md Outdated Show resolved Hide resolved
@florian-h05
Copy link
Author

florian-h05 commented Jul 24, 2022

@rkoshak
Sorry that I did not react to your review for some time, I was very busy last week and focused on my work at a new MainUI component/widget, but this should be finished soon and then I can continue to work on the docs.

@rkoshak
Copy link
Owner

rkoshak commented Jul 26, 2022

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 <florianh_dev@icloud.com>
@florian-h05
Copy link
Author

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.

Copy link
Owner

@rkoshak rkoshak left a 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.

rules/concepts.md Outdated Show resolved Hide resolved
rules/concepts.md Outdated Show resolved Hide resolved
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
@florian-h05
Copy link
Author

I addressed your last review, should be ready for merging.

How/Where to proceed with the triggers (1b of your issue) and so on?
Also in this file?

@florian-h05 florian-h05 changed the title [WIP] Add Rules Concepts page Add Rules Concepts page Aug 6, 2022
@rkoshak rkoshak merged commit 6512d20 into rkoshak:rules-rework Aug 8, 2022
@rkoshak
Copy link
Owner

rkoshak commented Aug 8, 2022

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.

@florian-h05
Copy link
Author

Perfect. Let's roll with it. Thanks for your work on this and putting up with my nit picking.

You are welcome!

Currently I am on vacation, but when I am back home I am planning to continue work on the docs.
I think that I will take 1b, 1c and 1d of openhab#1855.

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 :)

@florian-h05 florian-h05 deleted the rules-rework-1 branch August 15, 2022 17:27
@rkoshak
Copy link
Owner

rkoshak commented Aug 15, 2022

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.

@florian-h05
Copy link
Author

Looks like the dandy checklist TODO here can't be combined with a table so we'll just have to manage them individually.

I‘ve seen your todo column at the issue, might be better using the ✅ emoji.

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.

I thought that we would do it that way, but explanation is always good.

The fact that we are going up to 8 pages total is already causing me some concerns.

That‘s much, you are right, but as we write about many things I am okay with that .

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.

2 participants