Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move away from naive datetimes in entry fields, introduce pendulum #3758

Merged
merged 25 commits into from Dec 24, 2023

Conversation

gazpachoking
Copy link
Member

@gazpachoking gazpachoking commented May 3, 2023

Motivation for changes:

Improve the experience when dealing with dates in templates and if plugin:

  • Have an expected fallback when using a combination of naive and tz aware datetimes instead of failing.
  • Stop dropping all timezone info from our datetimes so time comparisons "just work" no matter the timezone.
  • Have an (unsuprising?) fallback when using a combination of naive and tz aware datetimes instead of failing.
  • Make dealing with timezones easy when used in templates.
  • Start using pendulum to make our internal handling of timezones and datetimes easier, as well as provide some nice syntax sugar in templates. e.g.
if:
- "transmission_date_done < now.subtract(days=60)": accept # no timedelta needed!

In a notification template (or for use in logging) Torrent finished {{transmission_date_done.diff_for_humans()}} would turn into something like Torrent finished 2 months ago

Detailed changes:

  • now and utcnow in templates are timezone aware, but when compared to naive times they assume the naive times are in the same timezone as them. This both replicates the current naive behavior, but also allows the user to be explicit about timezones if they wish. e.g. some_anime_release_date > now.in_timezone('Asia/Tokyo')
  • Plugins providing entry fields representing actual datetimes should now provide timezone aware datetimes.
  • Plugins providing plain dates without a time (like release dates) should either provide a date, or a naive datetime at midnight. When being set on an entry, plain dates will be turned into naive datetimes at midnight.
  • Entry will cast any stdlib datetimes or dates set on entry fields to our magic DateTimes, this adds convenience methods and makes dealing with them much easier. Translating dates -> DateTimes means they can then be compared directly to 'now'. It also adds the same semantics as our magic nows, where they can still be compared naive<>tz aware
  • Since all entry fields are guaranteed to be pendulum DateTimes, plugins are free to use the pendulum methods when dealing with datetime entry fields.
  • Drops python 3.7, as new pendulum doesn't support it and it's EOL

To Do:

  • Solicit feedback about the general idea as well as the approach.
  • Update as many plugins as possible (and appropriate) to provide tz aware datetimes.
  • Bump version and put a warning about possible differences.
  • Add some tests to make sure everything is working as expected (especially the magic now).
  • Looks like pendulum doesn't do so well with subclassing right now. Allow subclassing DateTimes to work as expected sdispater/pendulum#707

@gazpachoking
Copy link
Member Author

gazpachoking commented May 5, 2023

Gah. We are using datetimes for a lot of stuff that is actually just dates (mostly things like release dates.) Adding a timezone to something like that doesn't make a lot of sense, because we already don't know on what time that day the thing was released, and also don't know what timezone it was in. I'm tempted to make all these fields actual dates rather than datetimes. The problem would be that it could break people's if statements because they couldn't be compared to now anymore. (although, just straight up comparing to now is already 'wrong' by up to 24+ hours.)

EDIT: Updated the plan for these in the OP.

@gazpachoking gazpachoking changed the title Move away from naive datetimes, introduce pendulum Move away from naive datetimes in entry fields, introduce pendulum May 5, 2023
@gazpachoking
Copy link
Member Author

Ooh. My pendulum change went in, this might be back on the table when they make a release.

@paranoidi
Copy link
Member

Sweet :)

@gazpachoking
Copy link
Member Author

I made some changes to how things worked a bit. Now all datetimes (and dates) that go into entry fields are made into CoercingDateTimes, which are Pendulum DateTimes, with the added property that they are allowed to compare between tz aware and naive datetimes. This is to make transitioning from our fully naive past to fully tz aware future as painless as possible. I also adjusted how that class works, so instead of making comparisons always be naive, it assumes the other datetime is in the same timezone (very similar effect, maybe identical?)

@gazpachoking
Copy link
Member Author

The test failure is to do with using the git version in our deps atm. We will wait for a real release before merging.

@gazpachoking gazpachoking marked this pull request as ready for review December 17, 2023 19:54
@gazpachoking
Copy link
Member Author

Okay. I think pendulum and this PR are in a state we could release this now. Just need to bump version and write some docs/warnings if we want to go with it.

@gazpachoking gazpachoking merged commit c01d313 into develop Dec 24, 2023
8 checks passed
@gazpachoking gazpachoking deleted the pendulum branch December 24, 2023 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants