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 SensorEntityClass support for Environment Canada integration #58615

Merged
merged 10 commits into from
Jan 7, 2022
Merged

Add SensorEntityClass support for Environment Canada integration #58615

merged 10 commits into from
Jan 7, 2022

Conversation

gwww
Copy link
Contributor

@gwww gwww commented Oct 28, 2021

Breaking change

  • The sensors no longer have Timestamp as an extra state attribute. Use the new timestamp sensor that can be used in automations that track state changes.
  • The sensors alert sensors (advisories, endings, statements, warnings, and watches) state is now a count of the number of active alerts for the sensor type and the extra state attributes contain the alert text and alert timestamp. The attributes are named alert_<x> and alert_time_<x> where is 1 for alert 1, 2 for alert 2, etc.

Proposed change

Add SensorEntityClass support for Environment Canada integration. This PR also addresses a comment by @MartinHjelmare on the previous Environment Canada PR (#57746).

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

This PR is a follow on to #57746.

With the intent of keeping this PRs small as possible, there will be at least one more PR with some additional cleanup and adding another sensor that requires another Coordinator.

No document updates required for this PR.

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

@probot-home-assistant
Copy link

Hey there @michaeldavie, mind taking a look at this pull request as it has been labeled with an integration (environment_canada) you are listed as a code owner for? Thanks!
(message by CodeOwnersMention)

@gwww gwww mentioned this pull request Oct 28, 2021
22 tasks
@MatthewFlamm
Copy link
Contributor

The sensors alert sensors (advisories, endings, statements, warnings, and watches) state is now a count of the number of active alerts for the sensor type and the extra state attributes contain the alert text and alert timestamp. The attributes are named alert_ and alert_time_ where is 1 for alert 1, 2 for alert 2, etc.

Is this type of sensor allowed now? There was a discussion of this in #37415 (comment) where a geo_location model was preferred. I never implemented as the geo_location model is too complex for my tastes and time.

@gwww
Copy link
Contributor Author

gwww commented Oct 28, 2021

I have to admit I don't find that the solution in the PR isn't the best, but it is the best that a can see right now. I'm not sure firing events is what users want. Storing the alerts in the extra state attributes makes them always available. As @cgarwood says in his comment in the PR you reference: HA doesn't really have a good system for weather alerts. I don't know of anything that's changed, so I'm proposing that we listen to options and if nothing compelling go ahead as is.

If HA comes up with a better solution as some point I'd be happy to help implement and fix up this integration.

Last bit.. in the referenced PR it seems part of the discomfort is that events and state change were being done and it was asked to pick one of the two. This PR has only state change.

@MatthewFlamm
Copy link
Contributor

I mainly raised this issue so that I can also get clarity for implementing something similar for NWS integration. The sensor only approach has a lot of downsides from a user experience perspective. A new entity type for these kind of alerts would be preferred IMO.

@michaeldavie
Copy link
Contributor

I've tested this branch locally and it's working well. I like the addition of the observation time sensor.

@gwww
Copy link
Contributor Author

gwww commented Nov 5, 2021

Any chance someone could take a look at this one? Thx!

@gwww
Copy link
Contributor Author

gwww commented Nov 19, 2021

Would it be possible for someone to take a look at this?

@frenck frenck added the smash Indicator this PR is close to finish for merging or closing label Nov 29, 2021
homeassistant/components/environment_canada/const.py Outdated Show resolved Hide resolved
homeassistant/components/environment_canada/const.py Outdated Show resolved Hide resolved
homeassistant/components/environment_canada/const.py Outdated Show resolved Hide resolved
homeassistant/components/environment_canada/const.py Outdated Show resolved Hide resolved
homeassistant/components/environment_canada/const.py Outdated Show resolved Hide resolved
homeassistant/components/environment_canada/const.py Outdated Show resolved Hide resolved
homeassistant/components/environment_canada/const.py Outdated Show resolved Hide resolved
homeassistant/components/environment_canada/const.py Outdated Show resolved Hide resolved
homeassistant/components/environment_canada/const.py Outdated Show resolved Hide resolved
Copy link
Member

@frenck frenck left a comment

Choose a reason for hiding this comment

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

Thanks, @gwww 👍

@frenck frenck merged commit 1284337 into home-assistant:dev Jan 7, 2022
@gwww
Copy link
Contributor Author

gwww commented Jan 7, 2022

No, thank you!

If there are some boil plate changes that need to be made, I'd be happy to pick one up. I don't know where to look though.

@gwww gwww deleted the ec-sens branch January 7, 2022 17:37
@github-actions github-actions bot locked and limited conversation to collaborators Jan 8, 2022
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.

6 participants