Add optional location based region to dwd_weather_warnings #96027

andarotajo · 2023-07-06T21:04:29Z

Proposed change

This PR adds the optional feature to have a dynamic region by configuring it to use a device tracker that contains the current location. This allows the user to have weather warnings wherever they go instead of using fixed locations.

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)
Deprecation (breaking change to happen in the future)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request: Add device_tracker documentation to dwd_weather_warnings home-assistant.io#28107

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
I have followed the perfect PR recommendations
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:

Documentation added/updated for www.home-assistant.io

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.

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

home-assistant · 2023-07-06T21:04:38Z

Hey there @runningman84, @stephan192, @hummel95, mind taking a look at this pull request as it has been labeled with an integration (dwd_weather_warnings) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of dwd_weather_warnings can trigger bot actions by commenting:

@home-assistant close Closes the pull request.
@home-assistant rename Awesome new title Renames the pull request.
@home-assistant reopen Reopen the pull request.
@home-assistant unassign dwd_weather_warnings Removes the current integration label and assignees on the pull request, add the integration domain after the command.

homeassistant/components/dwd_weather_warnings/config_flow.py

home-assistant · 2023-07-07T12:55:01Z

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

github-actions · 2023-12-06T13:06:44Z

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.

github-actions · 2024-02-06T16:05:10Z

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.
If you are the author of this PR, please leave a comment if you want to keep it open. Also, please rebase your PR onto the latest dev branch to ensure that it's up to date with the latest changes.
Thank you for your contribution!

emontnemery · 2024-02-14T14:17:28Z

@andarotajo It's been concluded the approach you take in this PR is OK, but the PR needs a rebase as well as tests covering the new functionality. Please mark the PR ready for review once that's done.

andarotajo · 2024-02-15T20:19:27Z

... as well as tests covering the new functionality.

I just saw with the rebasing that I had added tests for the config flow GPS option in my initial commit. Does that not cover enough or what tests am I missing for this PR?

homeassistant/components/dwd_weather_warnings/__init__.py

homeassistant/components/dwd_weather_warnings/config_flow.py

homeassistant/components/dwd_weather_warnings/util.py

homeassistant/components/dwd_weather_warnings/sensor.py

homeassistant/components/dwd_weather_warnings/coordinator.py

homeassistant/components/dwd_weather_warnings/__init__.py

emontnemery · 2024-03-04T14:57:52Z

... as well as tests covering the new functionality.

I just saw with the rebasing that I had added tests for the config flow GPS option in my initial commit. Does that not cover enough or what tests am I missing for this PR?

It does not cover enough, only 75% of the added code is covered.

You can easily generate a coverage report when running pytest locally:

pytest --cov=homeassistant/components/dwd_weather_warnings--cov-report html tests/components/dwd_weather_warnings

andarotajo · 2024-03-06T18:57:05Z

I'm sorry I think I broke something while rebasing... Can I somehow salvage this? I followed the due to a conflict docs but probably missed something

Edit: Fixed it luckily...

homeassistant/components/dwd_weather_warnings/__init__.py

emontnemery · 2024-03-11T17:26:21Z

homeassistant/components/dwd_weather_warnings/__init__.py

+        try:
+            position = get_position_data(hass, device_tracker)
+        except (EntityNotFoundError, AttributeError) as err:
+            # The provided device_tracker is not available or missing required attributes.
+            LOGGER.error(f"Failed to setup dwd_weather_warnings: {repr(err)}")
+            return False

+        api = await hass.async_add_executor_job(DwdWeatherWarningsAPI, position)


All of this seems like duplicated logic which the coordinator also needs to handle, can you try to make it happen as a result of calling DwdWeatherWarningsCoordinator.async_config_entry_first_refresh instead?

I'll see what I can do, not familiar with that method though

Took a few tries but I got it (see last 3 commits)

homeassistant/components/dwd_weather_warnings/config_flow.py

emontnemery · 2024-03-11T17:33:40Z

homeassistant/components/dwd_weather_warnings/coordinator.py

@@ -23,4 +28,14 @@ def __init__(self, hass: HomeAssistant, api: DwdWeatherWarningsAPI) -> None:

    async def _async_update_data(self) -> None:
        """Get the latest data from the DWD Weather Warnings API."""
-        await self.hass.async_add_executor_job(self.api.update)
+        if device_tracker := self.config_entry.data.get(CONF_REGION_DEVICE_TRACKER):


I'd suggest to store device_tracker in the object instead, and just do like this here:

if self._device_tracker:

I'm not following completely. Do you mean having it defined in the __init__ as self.device_tracker = None and doing the initialization once in async_config_entry_first_refresh?

Edit: Nevermind got it

emontnemery · 2024-03-11T17:39:38Z

homeassistant/components/dwd_weather_warnings/config_flow.py

+            if all(k not in user_input for k in valid_options):
+                errors["base"] = "no_identifier"
+            elif all(k in user_input for k in valid_options):
+                errors["base"] = "ambiguous_identifier"


This seems a bit unnecessary. Maybe just do a set intersection and check the length?
Not sure if my suggestion is more readable though.

EXCLUSIVE_OPTIONS = {CONF_REGION_IDENTIFIER, CONF_REGION_DEVICE_TRACKER} options = user_input.keys() & EXCLUSIVE_OPTIONS if not options: ... elif len(options) == 2: ...

In my opinion the current solution is easier to understand though not as efficient probably

That's fine, it's a matter of taste 👍

emontnemery · 2024-03-18T15:08:36Z

homeassistant/components/dwd_weather_warnings/coordinator.py

+                DwdWeatherWarningsAPI, position
+            )
+        else:
+            if self._api is None:


Can this happen? self._api is unconditionally assigned in __init__

It might not be possible anymore. I think ruff marked this line at one point but I can't check for around 3 weeks as I'm on vacation as of today.

Will check after that of course

OK. I've marked the PR as draft, please mark it ready for review when you've had time to check 👍

Removed it in df914c2 :)

emontnemery · 2024-03-18T15:09:14Z

homeassistant/components/dwd_weather_warnings/coordinator.py

        """Initialize the dwd_weather_warnings coordinator."""
        super().__init__(
            hass, LOGGER, name=DOMAIN, update_interval=DEFAULT_SCAN_INTERVAL
        )

-        self.api = api
+        self._api = DwdWeatherWarningsAPI(None)


What's the meaning passing None here?

This initializes the API with an invalid state. I chose this instead of None to avoid having to deal with a potential None object in the sensor.py where the data is primarily used

I don't fully follow. The normal pattern for coordinator based integrations is that entities only access the coordinator after a successful refresh.

An option could be to not initialize self._api in __init__, instead just add a type annotation for it on class level:

_api: DwdWeatherWarningsAPI

If there's some bug causing sensors to access the coordinator before the first successful data update, an attribute error will be raised.

Updated the initialization of self._api in df914c2 as suggested

homeassistant/components/dwd_weather_warnings/exceptions.py

emontnemery · 2024-04-15T11:54:13Z

homeassistant/components/dwd_weather_warnings/sensor.py

@@ -93,18 +95,18 @@ def __init__(
            entry_type=DeviceEntryType.SERVICE,
        )

-        self.api = coordinator.api
+        self.api = coordinator._api


If the location is changed, the coordinator creates a new api object, I don't think it's valid to store a reference here, don't we need to access the api object via the coordinator instead?

A reference of a complex object shares the memory with the original as far as I know so this should be fine. But using self.coordinator._api directly instead of a reference is probably cleaner..

A reference of a complex object shares the memory with the original as far as I know so this should be fine

That's absolutely not the case, as evident from this snippet:

$ python3 Python 3.11.5 (main, Oct 19 2023, 09:15:06) [GCC 9.4.0] on linux Type "help", "copyright", "credits" or "license" for more information. >>> class Coordinator: ... api = object() ... >>> coordinator = Coordinator() >>> original_api = coordinator.api >>> coordinator.api = object() >>> original_api is coordinator.api False

Don't the added tests cover creating new API object when location changes?

The tests cover creating the API object initially but not a location change that would result in the API having a different city set.

Also noticed that using self.coordinator._api doesn't sit well with Pylint. Is renaming the _api variable to api the way to go here?

Yeah, name it api if it's to be used from other classes.

Updated in 848cfc5

emontnemery · 2024-04-15T14:13:45Z

homeassistant/components/dwd_weather_warnings/coordinator.py

+            self._api = await self.hass.async_add_executor_job(
+                DwdWeatherWarningsAPI, position
+            )


I think we should avoid creating a new object if the location has not changed.

Do you mean the region that the API would determine using the coordinates or the coordinates directly? I'd have to create the API object to test the former.

I meant that if the location of the device tracker has not changed, we should not create a new API object since it seems to be somewhat expensive? Or is the cost of just updating an existing API object similar to creating an API object, including the initial update?

The initialization includes 5 API calls to get the required information based on what the object was initialized with, i.e. region ID, region name or GPS coordinates. So it's definitely more costly as the updates only do a singular API call because the required information is already there.

Is there some convenient method to verify if the location has changed instead of having to store the previous position?

Just store the previous position. Maybe check if the position has changed more than 100m (or whatever is reasonable) to avoid recreating the API because of GPS inaccuracy. We have some helpers to help calculate the absolute distance between two points.

Implemented in 848cfc5

After a bit of googling I chose 50m as the threshold. I've also added a normal API update in case the object doesn't get recreated as the data wouldn't update otherwise

emontnemery · 2024-04-15T15:10:58Z

Please mark the PR ready for review when the requested changes have been implemented 👍

emontnemery

LGTM, thanks @andarotajo 👍

A follow-up PR which adds some additional tests of the coordinator would be great, in particular to assert a new API object is created when needed.

Stale

andarotajo · 2024-04-22T07:24:35Z

I'm pretty new to Python so thanks for all the help and patience!

I'll make a follow-up PR for the tests and for error handling with repair issues too when I've got the time 👍

home-assistant bot added cla-signed has-tests integration: dwd_weather_warnings new-feature by-code-owner Quality Scale: No score labels Jul 6, 2023

andarotajo mentioned this pull request Jul 6, 2023

Add device_tracker documentation to dwd_weather_warnings home-assistant/home-assistant.io#28107

Merged

8 tasks

andarotajo marked this pull request as ready for review July 6, 2023 21:27

MartinHjelmare requested changes Jul 7, 2023

View reviewed changes

homeassistant/components/dwd_weather_warnings/config_flow.py Show resolved Hide resolved

home-assistant bot marked this pull request as draft July 7, 2023 12:55

willkxx-e approved these changes Sep 4, 2023

View reviewed changes

MartinHjelmare self-assigned this Sep 7, 2023

github-actions bot added the stale label Dec 6, 2023

MartinHjelmare removed the stale label Dec 8, 2023

github-actions bot added the stale label Feb 6, 2024

MartinHjelmare removed the stale label Feb 6, 2024

emontnemery mentioned this pull request Feb 14, 2024

Document integration can consume the state of entities which provide location home-assistant/developers.home-assistant#2081

Merged

5 tasks

andarotajo force-pushed the dwd_add_gps_option branch from 0fba940 to 3e2a00e Compare February 15, 2024 19:46

emontnemery changed the title ~~Add GPS tracking support~~ Add optional location based region to dwd_weather_warnings Feb 16, 2024

emontnemery requested changes Feb 16, 2024

View reviewed changes

MartinHjelmare previously requested changes Feb 16, 2024

View reviewed changes

MartinHjelmare removed their assignment Feb 16, 2024

home-assistant bot requested review from MartinHjelmare and emontnemery March 6, 2024 21:12

emontnemery requested changes Mar 11, 2024

View reviewed changes

home-assistant bot marked this pull request as draft March 11, 2024 17:40

andarotajo added 4 commits March 14, 2024 19:27

Clean up code

b3c5908

Consolidate duplicate code and adjust tests

13e1651

Fix runtime error

8edddbe

Fix blocking of the event loop

345ee77

andarotajo marked this pull request as ready for review March 14, 2024 22:18

home-assistant bot requested a review from emontnemery March 14, 2024 22:19

emontnemery reviewed Mar 18, 2024

View reviewed changes

emontnemery marked this pull request as draft March 19, 2024 11:04

Adjust API object handling

df914c2

andarotajo marked this pull request as ready for review April 13, 2024 07:25

home-assistant bot requested a review from emontnemery April 13, 2024 07:25

emontnemery reviewed Apr 15, 2024

View reviewed changes

homeassistant/components/dwd_weather_warnings/exceptions.py Outdated Show resolved Hide resolved

Update homeassistant/components/dwd_weather_warnings/exceptions.py

f7f1ce3

emontnemery reviewed Apr 15, 2024

View reviewed changes

emontnemery marked this pull request as draft April 15, 2024 15:11

Optimize coordinator data update

848cfc5

andarotajo marked this pull request as ready for review April 19, 2024 18:32

home-assistant bot requested a review from emontnemery April 19, 2024 18:32

emontnemery approved these changes Apr 22, 2024

View reviewed changes

emontnemery merged commit 70d4b4d into home-assistant:dev Apr 22, 2024
24 checks passed

github-actions bot locked and limited conversation to collaborators Apr 23, 2024

andarotajo deleted the dwd_add_gps_option branch April 29, 2024 18:10

Add optional location based region to dwd_weather_warnings #96027

Add optional location based region to dwd_weather_warnings #96027

Conversation

andarotajo commented Jul 6, 2023 • edited Loading

Proposed change

Type of change

Additional information

Checklist

home-assistant bot commented Jul 6, 2023

home-assistant bot commented Jul 7, 2023

github-actions bot commented Dec 6, 2023

github-actions bot commented Feb 6, 2024

emontnemery commented Feb 14, 2024 • edited Loading

andarotajo commented Feb 15, 2024

emontnemery commented Mar 4, 2024

andarotajo commented Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andarotajo Mar 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emontnemery Mar 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emontnemery Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

andarotajo Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

emontnemery Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

andarotajo Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emontnemery Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emontnemery commented Apr 15, 2024

emontnemery left a comment

Choose a reason for hiding this comment

andarotajo commented Apr 22, 2024

andarotajo commented Jul 6, 2023 •

edited

Loading

emontnemery commented Feb 14, 2024 •

edited

Loading

andarotajo commented Mar 6, 2024 •

edited

Loading

andarotajo Mar 11, 2024 •

edited

Loading

emontnemery Mar 18, 2024 •

edited

Loading

emontnemery Apr 15, 2024 •

edited

Loading

andarotajo Apr 15, 2024 •

edited

Loading

emontnemery Apr 15, 2024 •

edited

Loading

andarotajo Apr 15, 2024 •

edited

Loading

emontnemery Apr 15, 2024 •

edited

Loading