Make async_track_time_change smarter #17199
Conversation
ghost
assigned
homeassistant/helpers/event.py
Outdated
| parameter = float(parameter[1:]) | ||
| return lambda time: time % parameter == 0 | ||
| res = [x for x in range(min_value, max_value + 1) | ||
| if x % parameter == 0] |
There was a problem hiding this comment.
Not sure why this is a float (cron syntax doesn't allow it), but I added it here to prevent a potential breaking change.
homeassistant/helpers/event.py
Outdated
| return res | ||
|
|
||
|
|
||
| def _find_next_time_expression_time(now, seconds, minutes, hours): |
There was a problem hiding this comment.
I think that is a utils.dt function
OttoWinter
changed the title
OttoWinter
force-pushed
the
new-async_track_time_change
branch
from
b2b7b55
to
445f588
Compare
homeassistant/helpers/event.py
Outdated
| now = dt_util.as_local(now) | ||
| # Prevent "can't compare offset-naive and offset-aware" errors, | ||
| # let's make everything timezone-aware | ||
| if now.tzinfo is None: |
There was a problem hiding this comment.
this can't happen. time changed events are send by the timer and are always UTC?
There was a problem hiding this comment.
Ah yes missed that. The tests were failiing because of it and I didn't investigate it in the timer source, but here.
homeassistant/helpers/event.py
Outdated
| # let's make everything timezone-aware | ||
| if now.tzinfo is None: | ||
| # pylint: disable=no-value-for-parameter | ||
| now = pytz.UTC.localize(now) |
There was a problem hiding this comment.
We should also use our own helpers in util.dt.
There was a problem hiding this comment.
The problem is that as_utc would in this case first make the dt object timezone-aware with the "default" timezone and then convert it back to utc. So if you input datetime(8.10.2018 14:31, tzinfo=None) it might output datetime(8.10.2018 12:31, tzinfo=UTC)
There was a problem hiding this comment.
well I guess it doesn't matter as we can remove this whole if-statement 😄
|
|
||
| self.assertRaises(ValueError, dt_util.parse_time_expression, 61, 0, 60) | ||
|
|
||
| def test_find_next_time_expression_time_basic(self): |
There was a problem hiding this comment.
One day it's time you learn how to write the modern style tests 😉
There was a problem hiding this comment.
Testing? That's overrated 😉 Just write a bunch of changes, publish a new version and hope nothing breaks #esphomelib 😅
I really should learn that :)
There was a problem hiding this comment.
async def test_something(hass):
assert len(hass.states.async_all()) == 0
that's it :)
homeassistant/util/dt.py
Outdated
| @@ -1,10 +1,14 @@ | |||
| """Helper methods to handle the time in Home Assistant.""" | |||
| import datetime as dt | |||
| import re | |||
| from typing import Any, Dict, Union, Optional, Tuple # noqa pylint: disable=unused-import | |||
| from typing import Any, Union, Optional, Tuple, \ | |||
Description:
Implement algorithm from home-assistant/architecture#88 into events base.
This will enable:
5/10), the use of lists and ranges (0,5,8-24). I'm planning to copy some of esphomeyaml's parser code to here.day_of_weekfor time triggers.This does not:
async_track_time_changemore efficient. Or at least not much. The listener still fires for each second.This does:
day,monthandyearkwargs fromasync_track_utc_time_change. They can be re-added later but they weren't used anywhere except for the tests.Questions:
event.pymight not be the best place for_find_next_time_expression_timeas it doesn't have too much to do with events. Should I move it todt.py?Checklist:
tox. Your PR cannot be merged unless tests passIf the code does not interact with devices: