Avoid a naive datetime.now() in buienradar - #178368
Conversation
The check only suppresses the parse warning during the midnight hour, so it wants the local hour. dt_util.now().hour expresses that directly, without formatting a naive datetime and parsing the result back to an int. Part of home-assistant/epics#117
home-assistant
Bot
added
bugfix
cla-signed
integration: buienradar
small-pr
|
Hey there @mjj4791, @ties, @RobBie1221, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
tests/components/buienradar/test_util.py:22
- Rename the first parameter ID to reflect that it tests a non-midnight hour;
05:30 UTCis23:30in America/Regina, sodaytimemislabels the scenario.
ids=["daytime", "midnight_hour"],
The midnight-hour branch had no test, and the change is not a pure refactor: datetime.now() read the system time zone while dt_util.now() reads the one configured in Home Assistant. The test pins the configured zone to America/Regina so the two disagree, and fails on dev. Part of home-assistant/epics#117
2ef417d to
b7fbb95
Compare
|
looks good to me |
| _LOGGER.debug("Buienradar parsed data: %s", result) | ||
| if result.get(SUCCESS) is not True: | ||
| if int(datetime.now().strftime("%H")) > 0: # pylint: disable=home-assistant-enforce-naive-now | ||
| if dt_util.now().hour > 0: |
There was a problem hiding this comment.
I think the intention is to suppress warnings during the midnight hour specifically in Europe (Netherlands) since that's where the Buienradar service is located.
The commit message that added this condition said:
Suppress an error that often occers between 00:00 and 01:00 CE(S)T during that time, probably because buienradar.nl is then updating its forcast for the next day. The API does not always work between these times (in the middle of the night).
So the correct adjustment would be to check the time in that timezone specifically, regardless of the system time or configured local time.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
The warning is suppressed between 00:00 and 01:00 because buienradar.nl updates its forecast then, so the hour that decides it belongs to the service rather than to the user's configured time zone. Tests pin both edges of the quiet hour in CET, repeat it in CEST so the offset is not assumed, and cover the case where it is the quiet hour where the user lives but not in Amsterdam.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (2)
tests/components/buienradar/test_util.py:52
- Condense this docstring to state only the non-obvious timezone choice. The parameter IDs and values already document the boundary and DST cases, so the detailed narration duplicates straightforward test data.
"""Test the parse failure warning is suppressed in the midnight hour.
buienradar.nl serves no data while it updates its forecast between 00:00 and
01:00 CE(S)T, so the warning is only interesting outside that hour. The hour
that decides this belongs to the service, so the configured time zone here is
homeassistant/components/buienradar/util.py:166
- Align the timezone behavior with the PR description. This explicitly uses Europe/Amsterdam, while the description says the warning window should follow Home Assistant's configured timezone via
dt_util.now(), so the implemented behavior and advertised behavior differ for users outside that zone; either use the configured timezone or update the PR rationale to establish the service timezone as the requirement.
service_tz = await dt_util.async_get_time_zone(SERVICE_TIME_ZONE)
if service_tz is None or dt_util.utcnow().astimezone(service_tz).hour > 0:
| await hass.config.async_set_time_zone("America/Regina") | ||
| freezer.move_to(frozen_time) | ||
|
|
||
| data = BrData(hass, {CONF_LATITUDE: 51.5, CONF_LONGITUDE: 5.5}, 60, []) |
There was a problem hiding this comment.
This isn't an isolated utility, despite its location. It (incorrectly) stores entities on its instance.
So we should not test it directly but indirectly by setting up the config entry, while patching the client library, and then asserting the log messages after moving time forward to force an update.
Proposed change
This check exists to suppress the parse warning during the midnight hour, when
Buienradar is known to return no data, so what it wants is the local hour.
dt_util.now().hoursays that directly, instead of formatting a naive datetimeto
%Hand parsing the string back into an int. The module already importsdt_utiland uses it a few lines above.Not a pure refactor
Worth calling out:
datetime.now()reads the system time zone, whiledt_util.now()reads the one configured in Home Assistant. For an instancewhere those two differ, the hour the warning is suppressed in moves from the
host's midnight to the user's. That looks like the intended behaviour to me —
the suppression is about when Buienradar stops serving data locally — but it is
a behaviour change, so I would rather flag it than bury it.
There was no test on this branch at all. The new one pins the configured zone
to
America/Reginaso it disagrees with the host, which makes it fail ondevand pass here.
Type of change
Additional information
datetime.now()in buienradar #177794datetime.now()calls epics#117Checklist
ruff format 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.To help with the load of incoming pull requests: