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 configuration flow for Buienradar integration #37796

Merged
merged 58 commits into from
May 4, 2021

Conversation

RobBie1221
Copy link
Contributor

@RobBie1221 RobBie1221 commented Jul 12, 2020

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:

  • The 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.
  • Weather, sensor and camera domain are setup in one ConfigEntry. For new entries, sensor and camera entities are disabled by default.
  • Dimension for camera images is fixed to 700 pixels

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

  • 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

Example entry for configuration.yaml:

# Example configuration.yaml

Additional information

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.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

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

@RobBie1221
Copy link
Contributor Author

Work in progress, config flow is fully implemented. Todo is change the current tests to work again and add tests for the configuration flow

@RobBie1221 RobBie1221 marked this pull request as ready for review July 13, 2020 20:05
@RobBie1221
Copy link
Contributor Author

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.

@RobBie1221
Copy link
Contributor Author

@mjj4791 @ties Mind taking a look at this as well?

homeassistant/components/buienradar/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/buienradar/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/buienradar/config_flow.py Outdated Show resolved Hide resolved
tests/components/buienradar/test_config_flow.py Outdated Show resolved Hide resolved
tests/components/buienradar/test_config_flow.py Outdated Show resolved Hide resolved
tests/components/buienradar/test_config_flow.py Outdated Show resolved Hide resolved
Copy link
Contributor

@OnFreund OnFreund left a 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.

@stale
Copy link

stale bot commented Oct 4, 2020

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.
Thank you for your contributions.

@stale stale bot added the stale label Oct 4, 2020
@RobBie1221
Copy link
Contributor Author

Waiting review, not stale

@stale stale bot removed the stale label Oct 4, 2020
@RobBie1221 RobBie1221 force-pushed the buienradar_cfg_flow branch 2 times, most recently from 7deb1f0 to 4a1214d Compare October 11, 2020 20:28
Copy link

@Kars-de-Jong Kars-de-Jong left a 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 ;-)

homeassistant/components/buienradar/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/buienradar/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/buienradar/config_flow.py Outdated Show resolved Hide resolved
@github-actions

This comment has been minimized.

@github-actions github-actions bot added the stale label Jan 28, 2021
@ties
Copy link
Contributor

ties commented Jan 28, 2021

I think this still looks relevant functionally. @RobBie1221?

@github-actions github-actions bot removed the stale label Jan 28, 2021
@RobBie1221
Copy link
Contributor Author

Yes, just waiting for a review.

@FeikoJoosten
Copy link

@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.

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.

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 🎉

@frenck frenck added the second-opinion-wanted Add this label when a reviewer needs a second opinion from another member. label Apr 30, 2021
homeassistant/components/buienradar/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/buienradar/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/buienradar/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/buienradar/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/buienradar/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/buienradar/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/buienradar/weather.py Outdated Show resolved Hide resolved
tests/components/buienradar/test_camera.py Outdated Show resolved Hide resolved
tests/components/buienradar/test_sensor.py Outdated Show resolved Hide resolved
@MartinHjelmare MartinHjelmare removed the second-opinion-wanted Add this label when a reviewer needs a second opinion from another member. label Apr 30, 2021
@RobBie1221
Copy link
Contributor Author

I think I got them all :-)

Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Great!

@MartinHjelmare MartinHjelmare merged commit c063f14 into home-assistant:dev May 4, 2021
Copy link
Member

@MartinHjelmare MartinHjelmare left a 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.

tests/components/buienradar/test_init.py Show resolved Hide resolved
tests/components/buienradar/test_sensor.py Show resolved Hide resolved
tests/components/buienradar/test_sensor.py Show resolved Hide resolved
tests/components/buienradar/test_weather.py Show resolved Hide resolved
tests/components/buienradar/test_init.py Show resolved Hide resolved
@RobBie1221 RobBie1221 deleted the buienradar_cfg_flow branch May 4, 2021 12:24
@RobBie1221
Copy link
Contributor Author

I'll address these in a follow-up PR

@RobBie1221 RobBie1221 mentioned this pull request May 5, 2021
21 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants