-
-
Notifications
You must be signed in to change notification settings - Fork 31
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
[rules] Better wording for time-related rule builder triggers #310
Comments
I like your proposal of making the time triggers more natural language-like! I’m fine with adding those, but I’d like to keep the existing ones besides them to avoid breaking changes. I guess it should be possible to just call the existing ones internally from the new ones (didn’t look at the code). However I’m currently not sure about the actual naming. For Item and Thing triggers it makes quite sense to use the past tense, because those triggers fire after the event has been received. I.e. „When time is …“, „When time (and date) matches Item …“, „When time matches cron …“ WDYT? |
Of course. I would remove them from docs though.
I agree that the present tense sounds better. I wasn't sure about that. So that would be:
One more thing: should cron trigger be |
👍 Keep them in JSDoc, but remove from README. Looks good to me!
Hmm, that's a difficult question. Cron expressions can be time only or date and time. I would vote for |
Item- and thing-related rule builders read naturally, for example (omitting dots and parentheses):
when item X changed to Y then ...
Time-related builders don't form English sentences:
.when().timeOfDay('17:00').then()
.when().cron('0 0 17 * * ?').then()
.when().dateTime('OutdoorLights_OffTime').timeOnly().then()
.when().dateTime('OutdoorLights_OffDate').then()
Those are logical from the programmer's point of view -- as they map directly to trigger types -- but they read a bit awkwardly IMO.
My proposal is to replace the above with something like:
.when().time().is('17:00').then()
or.when().time().was('17:00').then()
or.when().time().matched('17:00').then()
.when().time().matchedCron('0 0 17 * * ?').then()
.when().time().matchedItem('OutdoorLights_OffTime').then()
.when().timeAndDate().matchedItem('OutdoorLights_OffDate').then()
WDYT?
I'll be happy to create a PR.
The text was updated successfully, but these errors were encountered: