openhab / openhab-js

openHAB JavaScript Library for JavaScript Scripting Automation
https://www.openhab.org/addons/automation/jsscripting/
Eclipse Public License 2.0
38 stars 31 forks source link

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

Open mgr01 opened 6 months ago

mgr01 commented 6 months ago

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.

florian-h05 commented 6 months ago

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?

mgr01 commented 6 months ago

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 commented 6 months ago

I would remove them from docs though.

:+1: 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.