gpiozero icon indicating copy to clipboard operation
gpiozero copied to clipboard

Make TimeOfDay timezone aware - closes #1111

Open MusicalNinjaDad opened this issue 1 year ago • 8 comments

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 #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 #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

MusicalNinjaDad avatar Jan 30 '24 07:01 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?

lurch avatar Feb 03 '24 14:02 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?

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 avatar Feb 03 '24 19:02 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?

MusicalNinjaDad avatar Feb 03 '24 19:02 MusicalNinjaDad

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 :laughing:

lurch avatar Feb 04 '24 23:02 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?

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

MusicalNinjaDad avatar Feb 12 '24 13:02 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?

lurch avatar Feb 13 '24 08:02 lurch

Force push after rebasing onto latest gpiozero/gpiozero:master

MusicalNinjaDad avatar Feb 19 '24 14:02 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...

MusicalNinjaDad avatar Feb 19 '24 14:02 MusicalNinjaDad