Use separate data coordinators for AccuWeather observation and forecast #115628

bieniu · 2024-04-15T06:59:33Z

Breaking change

Proposed change

This PR:

adds separate data coordinator for observation and forecast
moves the data coordinator class to the coordinator module
removes options flow because we no longer need the option to enable forecast

The observation (current conditions) data update interval remains unchanged (every 40 minutes). Forecast data is updated every 6 hours. With this configuration, the integration performs 40 requests per day. This gives us 10 requests left for HA restarts or reload config entry (free AccuWeather accounts allow you to make 50 requests per day).

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:

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 Ruff (ruff format 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.

homeassistant/components/accuweather/coordinator.py

joostlek · 2024-04-15T19:18:07Z

homeassistant/components/accuweather/coordinator.py

+_LOGGER = logging.getLogger(__name__)
+
+
+class AccuWeatherDataUpdateCoordinator(TimestampDataUpdateCoordinator[Any]):


What's a TimestampDataUpdateCoordinator?

This is a DataUpdateCoordinator which keeps track of the last successful update:

core/homeassistant/helpers/update_coordinator.py

Lines 453 to 463 in c7e6f36

class TimestampDataUpdateCoordinator(DataUpdateCoordinator[_DataT]):

"""DataUpdateCoordinator which keeps track of the last successful update."""

last_update_success_time: datetime | None = None

@callback

def _async_refresh_finished(self) -> None:

"""Handle when a refresh has finished."""

if self.last_update_success:

self.last_update_success_time = utcnow()

WeaterEntity uses it:

core/homeassistant/components/weather/__init__.py

Lines 147 to 155 in c7e6f36

_DailyForecastUpdateCoordinatorT = TypeVar(

"_DailyForecastUpdateCoordinatorT", bound="TimestampDataUpdateCoordinator[Any]"

)

_HourlyForecastUpdateCoordinatorT = TypeVar(

"_HourlyForecastUpdateCoordinatorT", bound="TimestampDataUpdateCoordinator[Any]"

)

_TwiceDailyForecastUpdateCoordinatorT = TypeVar(

"_TwiceDailyForecastUpdateCoordinatorT", bound="TimestampDataUpdateCoordinator[Any]"

)

joostlek · 2024-04-15T19:18:30Z

homeassistant/components/accuweather/coordinator.py

+_LOGGER = logging.getLogger(__name__)
+
+
+class AccuWeatherDataUpdateCoordinator(TimestampDataUpdateCoordinator[Any]):


Can we make this a generic so we add the return type annotation?

joostlek · 2024-04-15T19:19:51Z

homeassistant/components/accuweather/sensor.py

+    for description in FORECAST_SENSOR_TYPES:
+        # Some air quality/allergy sensors are only available for certain
+        # locations.
+        if description.key not in forecast_daily_coordinator.data[description.day]:
+            continue
+        sensors.append(AccuWeatherSensor(forecast_daily_coordinator, description))


I think this can be a generator expression

joostlek · 2024-04-15T19:20:42Z

homeassistant/components/accuweather/weather.py

-class AccuWeatherEntity(
-    SingleCoordinatorWeatherEntity[AccuWeatherDataUpdateCoordinator]
-):
+class AccuWeatherEntity(CoordinatorWeatherEntity):


We can retain this coordinator type

I don't understand, why do you want to use SingleCoordinatorWeatherEntity?

I assumed the CoordinatorWeatherEntity also takes in a coordinator in the type CoordinatorWeatherEntity[AccuWeatherDataUpdateCoordinator] (but then the new coordinator)

homeassistant/components/accuweather/weather.py

home-assistant · 2024-04-15T19:22:29Z

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.

joostlek · 2024-04-16T10:03:06Z

tests/components/accuweather/test_sensor.py

@@ -42,12 +49,12 @@
 )


-async def test_sensor_without_forecast(
+async def test_sensor(


Snapshots would be a nice improvment for this test

As you wish 😄

joostlek

Oh I mean that as a separate PR, but this also works

bieniu · 2024-04-16T22:00:37Z

Thanks @joostlek

home-assistant bot added cla-signed has-tests integration: accuweather new-feature by-code-owner Quality Scale: platinum labels Apr 15, 2024

bieniu marked this pull request as ready for review April 15, 2024 07:08

joostlek reviewed Apr 15, 2024

View reviewed changes

homeassistant/components/accuweather/coordinator.py Outdated Show resolved Hide resolved

joostlek requested changes Apr 15, 2024

View reviewed changes

home-assistant bot marked this pull request as draft April 15, 2024 19:22

bieniu added 9 commits April 16, 2024 00:28

Remove forecast option

0cb8c49

Update strings

ed22b6e

Use separate DataUpdateCoordinator for observation and forecast

9829995

Fix tests

0fbf2b8

Remove unneeded variable

f31c741

Separate data coordinator classes

598091e

Use list comprehension

e5405eb

Separate coordinator clasess to add type annotations

aa39f41

Test the availability of the forecast sensor entity

a9721b5

bieniu force-pushed the accuweather-split-coordinator branch from ba30bac to a9721b5 Compare April 15, 2024 22:28

Add DataUpdateCoordinator types

6cf73af

bieniu marked this pull request as ready for review April 15, 2024 22:40

home-assistant bot requested a review from joostlek April 15, 2024 22:40

joostlek reviewed Apr 16, 2024

View reviewed changes

joostlek approved these changes Apr 16, 2024

View reviewed changes

Use snapshot for test_sensor()

bfc2da1

joostlek approved these changes Apr 16, 2024

View reviewed changes

bieniu merged commit e7076ac into home-assistant:dev Apr 16, 2024
24 checks passed

bieniu deleted the accuweather-split-coordinator branch April 16, 2024 22:00

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use separate data coordinators for AccuWeather observation and forecast #115628

Use separate data coordinators for AccuWeather observation and forecast #115628

bieniu commented Apr 15, 2024 •

edited

joostlek Apr 15, 2024

bieniu Apr 15, 2024 •

edited

joostlek Apr 15, 2024

joostlek Apr 15, 2024

joostlek Apr 15, 2024

bieniu Apr 15, 2024

joostlek Apr 15, 2024

home-assistant bot commented Apr 15, 2024

joostlek Apr 16, 2024

bieniu Apr 16, 2024

joostlek left a comment

bieniu commented Apr 16, 2024

		_LOGGER = logging.getLogger(__name__)


		class AccuWeatherDataUpdateCoordinator(TimestampDataUpdateCoordinator[Any]):

	class TimestampDataUpdateCoordinator(DataUpdateCoordinator[_DataT]):
	"""DataUpdateCoordinator which keeps track of the last successful update."""

	last_update_success_time: datetime \| None = None

	@callback
	def _async_refresh_finished(self) -> None:
	"""Handle when a refresh has finished."""
	if self.last_update_success:
	self.last_update_success_time = utcnow()

	_DailyForecastUpdateCoordinatorT = TypeVar(
	"_DailyForecastUpdateCoordinatorT", bound="TimestampDataUpdateCoordinator[Any]"
	)
	_HourlyForecastUpdateCoordinatorT = TypeVar(
	"_HourlyForecastUpdateCoordinatorT", bound="TimestampDataUpdateCoordinator[Any]"
	)
	_TwiceDailyForecastUpdateCoordinatorT = TypeVar(
	"_TwiceDailyForecastUpdateCoordinatorT", bound="TimestampDataUpdateCoordinator[Any]"
	)

Use separate data coordinators for AccuWeather observation and forecast #115628

Use separate data coordinators for AccuWeather observation and forecast #115628

Conversation

bieniu commented Apr 15, 2024 • edited

Breaking change

Proposed change

Type of change

Additional information

Checklist

Choose a reason for hiding this comment

bieniu Apr 15, 2024 • edited

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

home-assistant bot commented Apr 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joostlek left a comment

Choose a reason for hiding this comment

bieniu commented Apr 16, 2024

bieniu commented Apr 15, 2024 •

edited

bieniu Apr 15, 2024 •

edited