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

Added rule condition for not a holiday #2043

Merged
merged 1 commit into from
Feb 6, 2021

Conversation

soenkekueper
Copy link
Contributor

Fixes #2035
Signed-off-by: Sönke Küper soenkekueper@gmx.de

Signed-off-by: Sönke Küper <soenkekueper@gmx.de>
@soenkekueper soenkekueper requested a review from a team as a code owner January 1, 2021 15:21
@cweitkamp cweitkamp added rebuild Triggers the Jenkins PR build and removed rebuild Triggers the Jenkins PR build labels Jan 1, 2021
@soenkekueper
Copy link
Contributor Author

The jenkins build for this pull request was aborted after 120min, what could be the root cause of such a long running built?
I believe that this has nothing to do with my changes...
Is there anything i could do to get this ready to get merged?

@wborn wborn added rebuild Triggers the Jenkins PR build and removed rebuild Triggers the Jenkins PR build labels Jan 6, 2021
@wborn
Copy link
Member

wborn commented Jan 6, 2021

I've tuned the builds a bit recently, so let's see if it succeeds this time. 🙂

@soenkekueper
Copy link
Contributor Author

Nice, thanks!

@cweitkamp cweitkamp added automation enhancement An enhancement or new feature of the Core labels Jan 16, 2021
Base automatically changed from master to main January 18, 2021 20:04
@kaikreuzer
Copy link
Member

Looks good in general, but what I dislike a bit is the inherent negation - all the other ephemeris conditions don't do this.
I wonder, if a "is working day" condition would also fulfill your requirement? While it has a more complex meaning (as it combines weekday and holiday check), it might be the condition that most users actually need in their rules. Wdyt?

@soenkekueper
Copy link
Contributor Author

Hey,
i don't have any "is working day" condition at all. There is only an "it is a weekday" condition which is just the negation of "it is a weekend" but don't care about holiday. So which condition do you mean? When looking into the EphemerisConditionHandler and EphemerisManager.isBankHoliday(ZonedDateTime) it's just used by the "it is a holiday" condition.

I think you're right for most users a "is working day" condition would match, but i think the given "not a holiday" condition is more flexible, as some users may have to work on the weekend days too.

@kaikreuzer
Copy link
Member

My issue with "not a holiday" is that it is merely a negation of an existing condition and I'd somewhat want to avoid a proliferation of conditions that way. I agree that this is already the case with "is weekday" and "is weekend", although the negation is not so directly visible.
Ideally, I'd say the holiday condition should just have another config parameter to allow the negation. E.g. for item state conditions we also have "==" and "!=" as configuration options. Technically, this is definitely my preferred solution. The hardest thing about it in this case is to find a proper name for the enhanced condition, but I bet we can work something out.
@clinique As the master of Ephemeris, what's your view on this?

@clinique
Copy link
Contributor

clinique commented Feb 2, 2021

@clinique As the master of Ephemeris, what's your view on this?

I think that "is working day" or any equivalent would intuitively means neither weekend, neither holiday, so perhaps not very elegant but "not an holiday" feels less ambiguous.

Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

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

Ok, thanks for your feedback @clinique - then let's go for the less elegant, but also less ambiguous option. :-)

@kaikreuzer kaikreuzer merged commit 65218f8 into openhab:main Feb 6, 2021
@kaikreuzer kaikreuzer added this to the 3.1 milestone Feb 6, 2021
@soenkekueper soenkekueper deleted the 2035-NotAHoliday branch February 6, 2021 15:55
@soenkekueper
Copy link
Contributor Author

@kaikreuzer Thanks for merging.
There is also PR: openhab/openhab-webui#741 That adds the new Condition within the Ephemeris Quick Selection in the web-UI. Otherwise it's only available when choosing "show all".
Sorry i've missed to link this on creation.

splatch pushed a commit to ConnectorIO/copybara-hab-core that referenced this pull request Jul 11, 2023
Signed-off-by: Sönke Küper <soenkekueper@gmx.de>
GitOrigin-RevId: 65218f8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automation enhancement An enhancement or new feature of the Core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rule Engine: Condition for "it's NOT a holiday"
5 participants