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

[rules] Better wording for time-related rule builder triggers #310

Open
mgr01 opened this issue Dec 4, 2023 · 3 comments
Open

[rules] Better wording for time-related rule builder triggers #310

mgr01 opened this issue Dec 4, 2023 · 3 comments
Labels
enhancement New feature or request

Comments

@mgr01
Copy link

mgr01 commented Dec 4, 2023

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:

  1. .when().timeOfDay('17:00').then()
  2. .when().cron('0 0 17 * * ?').then()
  3. .when().dateTime('OutdoorLights_OffTime').timeOnly().then()
  4. .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:

  1. .when().time().is('17:00').then() or
    .when().time().was('17:00').then() or
    .when().time().matched('17:00').then()
  2. .when().time().matchedCron('0 0 17 * * ?').then()
  3. .when().time().matchedItem('OutdoorLights_OffTime').then()
  4. .when().timeAndDate().matchedItem('OutdoorLights_OffDate').then()

WDYT?

I'll be happy to create a PR.

@mgr01 mgr01 added the enhancement New feature or request label Dec 4, 2023
@florian-h05
Copy link
Contributor

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.
Time based triggers fire when their time condition is met, so probably the present tense should be used for them.

I.e. „When time is …“, „When time (and date) matches Item …“, „When time matches cron …“

WDYT?

@florian-h05 florian-h05 changed the title [rule builder] Better wording for time-related builders [rules] Better wording for time-related rule builder triggers Dec 5, 2023
@mgr01
Copy link
Author

mgr01 commented Dec 6, 2023

I’d like to keep the existing ones besides them to avoid breaking changes.

Of course. I would remove them from docs though.

Time based triggers fire when their time condition is met, so probably the present tense should be used for them.

I.e. „When time is …“, „When time (and date) matches Item …“, „When time matches cron …“

I agree that the present tense sounds better. I wasn't sure about that.

So that would be:

  1. .when().time().is('17:00').then()
  2. .when().time().matchesCron('0 0 17 * * ?').then()
  3. .when().time().matchesItem('OutdoorLights_OffTime').then()
  4. .when().timeAndDate().matchesItem('OutdoorLights_OffDate').then()

One more thing: should cron trigger be .when().time().matchesCron() or .when().timeAndDate().matchesCron()? It does have non-optional date part, right?

@florian-h05
Copy link
Contributor

I would remove them from docs though.

👍 Keep them in JSDoc, but remove from README.

Looks good to me!

One more thing: should cron trigger be .when().time().matchesCron() or .when().timeAndDate().matchesCron()? It does have non-optional date part, right?

Hmm, that's a difficult question. Cron expressions can be time only or date and time. I would vote for timeAndDate, even though for several cron expressions it would be only time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants