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 DateTime trigger #154

Merged
merged 5 commits into from
Aug 26, 2022
Merged

Conversation

florian-h05
Copy link
Contributor

@florian-h05 florian-h05 commented Aug 22, 2022

Fixes #153.

This adds support for the DateTime trigger, which was added in
openhab/openhab-core#2726.

@florian-h05 florian-h05 added the enhancement New feature or request label Aug 22, 2022
@florian-h05 florian-h05 added this to the 2.x.x milestone Aug 22, 2022
@florian-h05 florian-h05 requested a review from digitaldan August 22, 2022 22:04
@florian-h05 florian-h05 modified the milestone: 2.x.x Aug 22, 2022
This adds support for the DateTime trigger, which was added in
openhab/openhab-core#2726.

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
@florian-h05 florian-h05 force-pushed the add-datetime-trigger branch from c729e79 to 5d67dba Compare August 22, 2022 22:23
Copy link
Contributor

@digitaldan digitaldan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks! Only one small question on docs, but other then that looks good to merge.

@@ -22,8 +23,8 @@
* @property {string} oldStatus only for {@link triggers.ThingStatusChangeTrigger}: Previous state of Thing that triggered event
* @property {string} newStatus only for {@link triggers.ThingStatusChangeTrigger}: New state of Thing that triggered event
* @property {string} status only for {@link triggers.ThingStatusUpdateTrigger}: State of Thing that triggered event
* @property {string} eventType for all triggers except {@link triggers.PWMTrigger}, {@link triggers.PIDTrigger}: Type of event that triggered event (change, command, time, triggered, update)
* @property {string} triggerType for all triggers except {@link triggers.PWMTrigger}, {@link triggers.PIDTrigger}: Type of trigger that triggered event (for `TimeOfDayTrigger`: `GenericCronTrigger`)
* @property {string} eventType for all triggers except {@link triggers.PWMTrigger}, {@link triggers.PIDTrigger}, time triggers: Type of event that triggered event (change, command, time, triggered, update)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when we say "time triggers", do we mean TimeOfDayTrigger and DateTimeTrigger? If so, should we just call those out directly ?

Copy link
Contributor Author

@florian-h05 florian-h05 Aug 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We mean all three time triggers that we have:

  • TimeOfDayTrigger
  • GenericCronTrigger
  • DateTimeTrigger

Therefore, it is saving some space to just say time trigger, and if there are added any more time triggers in the future, we do not have to update the docs.

@florian-h05
Copy link
Contributor Author

@digitaldan
Can I merge?

@digitaldan
Copy link
Contributor

sorry, yes i'm fine, thanks!

@florian-h05 florian-h05 merged commit 46196f8 into openhab:main Aug 26, 2022
@florian-h05 florian-h05 deleted the add-datetime-trigger branch August 26, 2022 19:32
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

Successfully merging this pull request may close these issues.

[triggers] Add the new Time is <Item> trigger
2 participants