mfogel / django-timezone-field

A Django app providing DB, form, and REST framework fields for zoneinfo and pytz timezone objects.
BSD 2-Clause "Simplified" License
397 stars 95 forks source link

Use `zoneinfo.available_timezones` instead of `pytz.common_timezones` converted to `ZoneInfo` #98

Closed gdalmau closed 1 year ago

gdalmau commented 1 year ago

Description

In the attribute default_zoneinfo_tzs from TimeZoneField:

https://github.com/mfogel/django-timezone-field/blob/914e3ef979e00d819f74e3ae498a0f7b518556f1/timezone_field/fields.py#L41

Instead of converting all the pytz.common_timezones to ZoneInfo actually use zoneinfo.available_timezones().

Issue

There is a missmatch between zoneinfo.available_timezones() and pytz.common_timezones, which makes it not possible to use a timezone available in zoneinfo and not in pytz with this field.

Suggestion

import zoneinfo

class TimeZoneField(models.Field):

    ...

    default_zoneinfo_tzs = zoneinfo.available_timezones()
aleonsan commented 1 year ago

:+1: to the suggestion

mfogel commented 1 year ago

👍 as well to the suggestion. As django drops support for pytz, I would like this package to as well, and it order to do so it'll need to stop using pytz.common_timezones

I can see a three-part implementation process

gdalmau commented 1 year ago

👍 as well to the suggestion. As django drops support for pytz, I would like this package to as well, and it order to do so it'll need to stop using pytz.common_timezones

I can see a three-part implementation process

  • new minor version: add a new optional kwarg, something like use_zoneinfo_available_timezones with default value False
  • new major version: change default value of use_zoneinfo_available_timezones from False to True
  • new major version: drop kwarg use_zoneinfo_available_timezones altogether

Why not directly a major version that deprecates the usage of pytz? Django is a much bigger library which probably added the setting USE_PYTZ to ease the transition. In a library like this, I don't think such intermediate steps are needed.

mfogel commented 1 year ago

Why not directly a major version that deprecates the usage of pytz?

I had been thinking to hold off on releasing another major version for a bit. But now that I look at the complexity it'll add to add yet another kwarg I'm agreeing with you - I'd simpler to just do a new major version.

I'll release a final minor version without this change, and then plan for a new major version with this change.

gdalmau commented 1 year ago

@mfogel Other than the suggestion, do you think we need to change something else?

To put the suggestion made on a new minor release or a major release is debatable, we could argue that using ZoneInfo values when USE_DEPRECATED_PYTZ=False is expected, hence this is only a bug.

I think fully deprecating pytz from the library can wait and we can do this change regardless.

gdalmau commented 1 year ago

@mfogel thanks for the update