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

Use expected behvaior for above/below #7857

Merged
merged 1 commit into from Jun 2, 2017
Merged

Use expected behvaior for above/below #7857

merged 1 commit into from Jun 2, 2017

Conversation

emlove
Copy link
Contributor

@emlove emlove commented Jun 1, 2017

Description:

This PR makes the above/below conditions/triggers not treat equal as a match. This is what would be expected from the words above/below.

We are also considering adding new keywords min/max that use the old behavior, where an equal value is included. These are almost but not exactly the same. I'd like to get some opinions whether it's worth introducing the new keywords or not.

Related issue (if applicable): fixes #7170

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.github.io#<home-assistant.github.io PR number goes here>

Example entry for configuration.yaml (if applicable):

@mention-bot
Copy link

@armills, thanks for your PR! By analyzing the history of the files in this pull request, we identified @balloob, @fabaff and @pvizeli to be potential reviewers.

@balloob
Copy link
Member

balloob commented Jun 2, 2017

I'm not a too big fan of adding max and min, it might be confusing that we have 4 different options. Let's not add it for now and when a big amount of people complain we might reconsider.

@balloob
Copy link
Member

balloob commented Jun 2, 2017

Going to merge this, if we want to do min/max it can go in another PR.

@balloob balloob merged commit beb8c05 into home-assistant:dev Jun 2, 2017
@balloob balloob mentioned this pull request Jun 2, 2017
@point-4ward
Copy link
Contributor

point-4ward commented Jun 2, 2017

@balloob - when this is integrated in to a release can it be marked as a breaking change please?

Reason I ask:

I currently have a number of automations using the haveibeenpwned sensor, the alert is set to go off when the sensor reading is above: 1 which is compliant with the current format. This will need changing to above: 0 for the new format, and if I don't change it my email addresses could be compromised without the alert sounding, however if I change it now I will get false alerts.

I suspect that a number of people will be in this boat for one reason or another. I appreciate it's not an actual breaking change, but it is important that people know to change this number if it is critical to their automations/scripts/etc, and I only know about it because I commented in this thread, many other users won't.

@emlove emlove deleted the numeric-state-trigger branch June 2, 2017 15:26
@emlove
Copy link
Contributor Author

emlove commented Jun 2, 2017

@mf-social We do have the PR flagged with the "Breaking Change" label. It will be included in the release notes. I definitely agree is a breaking change that people need to know about.

@balloob Agree 100% with the notes about min/max.

@emlove emlove changed the title [WIP] Use expected behvaior for above/below Use expected behvaior for above/below Jun 2, 2017
@home-assistant home-assistant locked and limited conversation to collaborators Sep 4, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

numeric_state trigger "below" acting like "below or equal"
6 participants