-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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 configuration flow for Buienradar integration #37796
Add configuration flow for Buienradar integration #37796
Conversation
Work in progress, config flow is fully implemented. Todo is change the current tests to work again and add tests for the configuration flow |
This is basically done. See limitations in description, but first I'd like to get this approved. Also the weather writes to hass.data[buienradar_condition] which I think should be refactored to hass.data[DOMAIN]. The unload will most likely leave the data updater intact, which is not really nice when removing the integration without restart. I'll address this as well in a later PR. |
503435d
to
8307791
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, but keep in mind someone with write access will have to approve as well.
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
Waiting review, not stale |
7deb1f0
to
4a1214d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments about comments ;-)
697e0cf
to
bf00353
Compare
This comment has been minimized.
This comment has been minimized.
I think this still looks relevant functionally. @RobBie1221? |
Yes, just waiting for a review. |
@RobBie1221 would it be possible to add the proposed changes from my PR to this PR? I don't mind waiting till this PR is merged to do it myself, but since the changes are small I figured I might ask you for it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested again, still works great. Thanks, @RobBie1221 👍
Going to ask for a second look/opinion from a fellow reviewer, but I think it's great 🎉
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
I think I got them all :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I noticed some things that we should address in the tests. It seems there's no I/O directly during config entry setup but the platforms will eventually do I/O so it's safest to patch that regardless.
I'll address these in a follow-up PR |
Breaking change
This PR will deprecate yaml support for Buienradar integration and add UI configuration. The yaml configuration will be imported once. When users have a weather and sensor setup for same coordinates, they will be merged into one ConfigEntry. Only one camera configuration will be imported.
Following things are changed:
forecast
key of the weather platform is deprecated, forecast data is fetched in the same API call and to be in line with ADR-0003, all available data is exposed.Proposed change
This PR implements a basic configuration flow for the Buienradar integration. The integration currently includes the weather, camera and sensor platform. With the flow in this PR, you can setup all three in one configuration flow. The user can choose whether to include the radar image as camera, because this is fetched as image with a separate API call. The user is able to configure multiple instances as long as the lattitude/longitude is unique.
Furthermore, a "Name" attribute is now mandatory, which is used to create entities. In the Integrations page, the Buienradar card will show up with this configured name.
Setting options will be addressed in a later PR.
Type of change
Example entry for
configuration.yaml
:# Example configuration.yaml
Additional information
Checklist
black --fast homeassistant tests
)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest
.requirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
..coveragerc
.The integration reached or maintains the following Integration Quality Scale: