Make TimeOfDay timezone aware - closes #1111

[MusicalNinjaDad]

Background

utcnow() is deprecated in python3.12 to reduce the danger of naive utc times being incorrectly interpreted as local times. see the warning in the docs

The current implementation uses a flag, utc, in the instance to track whether the start and end times are naive utc or local time. This flag defaults to True.

This leads to deprecation warnings being raised which can only be dealt with in a non-thread-safe manner and collides with the future assumptions that users will make based upon the above deprecation.

Semantics of the change

I have tried to preserve the following semantics to ensure not only tehcnical but also logical backwards compatibility:

• default handling is for times to be treated as UTC if nothing else is specified
• utc=False is used for specifying that times are "local time"
• specifying utc=True will force a naive utc, raise a custom deprecation warning to better explain the situation and allow the official warning to propogate (providing the user with full context and avoiding threading issues)
• where a timezone is given which is not a fixed offset (e.g.: a location which may observe daylight saving time) the current time in the location today, right now is used for comparison

Additional points for discussion

1. It could be valuable to implement a new flag local (default: False) while still initially preserving the behaviour of utc. Given that utc is in future primarily used to flag local time TimeOfDay(time(7), time(8), local=True) would be more obvious in its intentions than TimeOfDay(time(7), time(8), utc=False). This would also allow for the utc flag to be moved to a less prominent place in the documentation, the arguments order and possibly fully deprecated in v3.0.
2. When providing a datetime instance as a start or end time the date is ignored and only the time element stored. This could be confusing, now that more use is made of the specifics of the start and end time objects (by leveraging their tzinfo). Any changes to this behaviour would significantly change the semantics of the function. Perhaps there is a way to allow for the user to decide whether they really mean "at this one specific point in time", or "at this time every day"? It may be worth formally documenting the possibility to provide a datetime object and highlighting this behaviour. (The ability to pass a datetime object is currently undocumented but tested).

Additional changes included

• adjustments to the validation process should now allow for substitution of datetime.datetime and datetime.time objects with others which provide the required interfaces but may not officially subclass the standard objects.
• adjustments the development environment to provide for virtual environments and running the tests on changes to every branch should contributors use these. (the commits from Support venv and use of github actions for contributors #1122)

Changes originally included but now removed from this PR based on comments

• fixes to the implementation of entry_points to ensure python3.12 compatibility. This effectively removes the need for Python 3.12 compatibility #1112. Given that the deprecation of utcnowoccurs in 3.12 it makes sense to include these changes here - otherwise testing in 3.12 will cause multiple tests to fail.

Closes #1111

[lurch]

I think this example of using different timezones for the start-time and end-time is a bit confusing?

[MusicalNinjaDad]

The general concept or the way the code is written? I felt the example was quite powerful, but maybe the initial paragraph above needs some work?
Would it be more readable as:

tz_LA = ZoneInfo('America/Los_Angeles')
tz_London = ZoneInfo('Europe/London')

LA_opens = time(8,30,tzinfo=tz_LA)
London_closes = time(18,00,tzinfo=tz_London)

officehours = TimeOfDay(LA_opens, London_closes)

[lurch]

I think the general concept of using different timezones for the start-time and end-time is a bit confusing.

I think your example assumes that the reader knows that the LA timezone is "negative" compared to the London timezone? And does it assume that LA doesn't close before London opens?

[lurch]

What backwards-compatibility implications does changing the default-value for utc have?

[MusicalNinjaDad]

Sematically, the behaviour is still the same: default behaviour is for the times to be treated as UTC. Technically, those will now be timezone-aware UTC times rather than naive ones.
TimeOfDay will still act exactly as before if the default is used, True is passed or False is passed.

The only risk I see is if someone has made some kind of use of the fact that previously the times were naive and had relied on implicity specifying utc rather than explicitly stating utc=True. I can't thin of any cases where someone would be doing something with the times which needed them to be naive, other than working in "local time".

Would it be worthwhile rephrasing self._aware = utc is None to something which takes a little more space and makes the behaviour more apparent. At the moment most of the relevant logic is later in _validate_time()

[lurch]

I think I'd need to do more reading up to understand the difference between "timezone-aware UTC times" and "naive UTC times" to understand what you're saying here; which I'm afraid I don't have time to do right now 😉

[MusicalNinjaDad]

@bennuttall , @waveform80 - the more I think about this; the more I think it would make sense to take the deprecation a step further and introduce a new boolean keyword argument local in addition to the changes to utc
I would move utc to the end of the signature and documentation, placing local more prominently.

This would make the normal usage:

1. use UTC times (in a timezone-aware fashion) = default
2. specify tzinfo in the construction of the start & end times to use a specific timezone
3. specify local=True to use local time

While still maintaining backwards compatibility, by keeping utc semantically identical to now.

@lurch - I'd also be interested in your views on this if you have any; your comments have been really useful in cleaning up rough edges.

[lurch]

I guess the "file reads" here doesn't make sense? (looks like a copy-paste typo?)

[MusicalNinjaDad]

We'd need to ask @bennuttall - it looks like he wrote that in #941

[lurch]

There seem to be a bunch of code-change overlaps with your entrypoint PR - perhaps those changes ought to be removed from this PR?

[MusicalNinjaDad]

There seem to be a bunch of code-change overlaps with your entrypoint PR - perhaps those changes ought to be removed from this PR?

Yes - That makes sense. I had built this on top of the python3.12 relevant bits, before beginning working on 3.7 compatibility.

I'll take them out of here

[MusicalNinjaDad]

As a general question @lurch - would you prefer individual commits to resolve each comment and then a re-squash once you're happy with the whole thing, or some other approach?

[lurch]

would you prefer individual commits to resolve each comment and then a re-squash once you're happy with the whole thing, or some other approach?

That's a question for Ben and Dave, the maintainers of the repo. I just try to help out by occasionally leaving (hopefully) helpful comments here and there 😆

[MusicalNinjaDad]

There seem to be a bunch of code-change overlaps with your entrypoint PR - perhaps those changes ought to be removed from this PR?

Yes - That makes sense. I had built this on top of the python3.12 relevant bits, before beginning working on 3.7 compatibility.

I'll take them out of here

Done and rebased - to make the future history more understandable

[lurch]

Perhaps it's also worth mentioning in the docstrings that if this is given a full datetime object, rather than just a time object, the "date" part of the datetime will be silently discarded, and only the "time" part will be used?

[MusicalNinjaDad]

Force push after rebasing onto latest gpiozero/gpiozero:master

[MusicalNinjaDad]

Perhaps it's also worth mentioning in the docstrings that if this is given a full datetime object, rather than just a time object, the "date" part of the datetime will be silently discarded, and only the "time" part will be used?

Yes - that's one for Ben & Dave as this is original functionality (see "Additional points for discussion - point 2) in the PR description for my view...