As we come closer to the deadline, I thought I'd like to bring attention to the one aspect of the zoneinfo-support PR that I am least certain about but which hasn't gotten much discussion, detailed here: https://github.com/django/django/pull/13877#issuecomment-758769902
*Note*: I don't think anything I'm talking about in this e-mail needs to block the existing PR! The tl;dr is that `timezone.make_aware` and some date truncation logic both take an `is_dst` parameter, which duplicates pytz's behavior. The current behavior as of 3.1 is that `is_dst` is a no-op unless you are using pytz. In the current version of the PR, `is_dst` is a no-op unless you are using pytz, but particularly in the case of date truncation logic, that *may* defeat some peoples' expectations. The options I see are: 1. Implement is_dst mostly like it's implemented in pytz-deprecation-shim, where explicitly passing `is_dst=None` will raise an exception on ambiguous and imaginary times, leaving it blank is a no-op, and specifying `True` or `False` sets the `fold` parameter to match what `pytz` would give you. If people are already able to use `make_aware` with non-pytz zones, this *may* introduce new exceptions into their code, which would not necessarily be backwards compatible. Depending on how broken support for non-pytz zones is "on the ground", this may not matter at all, since making theoretically backwards-incompatible changes to an otherwise broken function is not really backwards incompatible... 2. Implement is_dst, but require opt-in for the error-raising behavior: `is_dst=None` would raise errors only in the `pytz` case, `is_dst='error'` would always raise errors, and `is_dst=True` and `is_dst=False` would set the `fold` parameter to match what `pytz` would give you. This should be effectively backwards compatible, since leaving `is_dst` unspecified would keep the behavior as-is and specifying True/False with a non-pytz zone never worked anyway (and as such might be considered a bug anyway). 3. Leave as-is. External users who want to replicate `is_dst`-style behavior can use wrapper functions or subclasses to accomplish this anyway. I think the most annoying one to replicate would be date truncation logic <https://github.com/django/django/blob/8707e3f2dd07ab00ff33849615017c7a13bb64c6/tests/db_functions/datetime/test_extract_trunc.py#L1192-L1214>. People who want truncation with `is_dst` support would either want to wrap the output of their `.annotate` calls (e.g. `model = DTModel.objects.annotate(truncated_start=TruncDay('start_datetime')).get(); model.truncated_start = apply_is_dst(model.truncated_start, is_dst=True)`), or they would want to create a parallel truncation hierarchy subclassing from TruncBase, where is_dst support is added in convert_value <https://github.com/django/django/blob/83fcfc9ec8610540948815e127101f1206562ead/django/db/models/functions/datetime.py#L240-L259> (or TruncBase is monkey-patched to this effect). 4. Leave `make_aware` as is, but add `is_dst` support into TruncBase.convert_value, similar to what I'm doing in `django.forms.utils.from_current_timezone` — in that case we're just raising exceptions on error and not trying to re-implement `is_dst` in terms of `fold`, which is trickier than you'd think <https://pytz-deprecation-shim.readthedocs.io/en/latest/migration.html#semantic-differences-between-is-dst-and-fold>, but the implementation in pytz_deprecation_shim is pretty well-tested, so I am not too worried about it. Let me know what y'all want to do. I can also do this as a separate PR, as I think the "support zoneinfo" PR works as a standalone. One thing to consider is that even if we don't do this for 3.2, if 4.0 comes with a "hard switch" to zoneinfo, we'll need to decide what the behavior of `is_dst` is going to be, and what the future if `is_dst` looks like. One advantage that pytz_deprecation_shim has over Django is that it's replicating pytz's implementation, so it already needs a similar exception hierarchy to pytz's. Django would need to introduce its own exception hierarchy for "what happens when you try and create a datetime that is ambiguous or doesn't exist." There's also the problem that not all folds or gaps are created by DST transitions, and it's not entirely clear what `is_dst` should do in that case. Probably the easiest thing is to leave it in place and make it well-defined, or to deprecate `is_dst` in favor of a new argument like `preferred_fold` that uses the semantics of folds instead of the semantics of DST. Best, Paul On Tue, Jan 12, 2021 at 12:48 PM Paolo Melchiorre <pa...@melchiorre.org> wrote: > I agree with Adam. > > I've also left a small comment on the PR. > > Paolo > > On Tue, Jan 12, 2021 at 5:59 PM Adam Johnson <m...@adamj.eu> wrote: > > > > I think it's worth merging into 3.2. The change is quite small, the > potential benefits are quite large, and some users live LTS to LTS so could > be left without an option for a long time. > > > > I've left some review comments on the PR. > > > > On Tue, 12 Jan 2021 at 15:29, Paul Ganssle <p...@ganssle.io> wrote: > >> > >> Yeah, sorry I didn't get around to this until so close to the deadline. > December was a lot crazier for me than I had hoped. ☹ > >> > >> One thing I'll note is that this PR to allow using zoneinfo timezones > is mostly just adding tests. The substantive changes to the codebase are > very light, and there should be no behavioral change for users of pytz or > current users of other time zone providers (assuming such people exist). > There appears to be a nominal attempt to support non-pytz zones in > django.utils.timezones, so possibly this PR just improves support that was > already intended to be present in the first place. > >> > >> I do think it would be good to make it possible for people to do some > conversion over ahead of time if possible, particularly since no > deprecation warnings will be issued prior to the change away from pytz. > >> > >> Best, > >> Paul > >> > >> On 1/12/21 10:12 AM, Carlton Gibson wrote: > >> > >> Hi all. > >> > >> Paul Ganssle has followed up his earlier PoC PR, and the previous > discussion, with a smaller PR in order to allow using zoneinfo timezones > without an error: > >> > >> Add support for non-pytz zones > >> https://github.com/django/django/pull/13877 > >> > >> The idea is to target 3.2 for this. (Recall for 4.0 we'll switch to > zoneinfo, allowing folks to opt-in to the pytz compatibility shim, if > needed.) > >> > >> First glance, it looks OK, great even (absolutely as expected from > Paul). > >> > >> I don't have capacity to properly sit down with it before the feature > freeze (due Thursday) though. > >> So if it were just me, I'd have to say it's too hot, too close to the > deadline, and we should just target 4.0 > >> > >> However, maybe others, and the Technical Board who have the authority > to specify, think that this is something we should take the extra time if > needed and make sure we get in for 3.2? > >> (Any extra eyes reviewing would help in that case.) > >> > >> As per Aymeric's point on the previous discussion, it's likely this > doesn't affect most of our users. > >> Paul's concern is that Django incompatibility (along with Pandas he > mentioned in discussion) is blocking adoption throughout the community. > >> That's not necessarily our problem, but maybe this is a small enough > change that we should be flexible here? > >> With 3.2 being the next LTS, it'll be around for a while (which is > relevant I think). > >> > >> Can I ask for opinions please? Again, the option is to say that we'll > squeeze this in if review is fine. > >> The feature freeze isn't cast in stone, but we do have to draw the line > somewhere. > >> This one is sufficiently interesting that Mariusz and I don't want to > just decide with consultation. > >> > >> Thanks. > >> > >> Kind Regards, > >> > >> Carlton > >> > >> > >> > >> > >> On Monday, 4 January 2021 at 10:56:42 UTC+1 Carlton Gibson wrote: > >>> > >>> Hi all, > >>> > >>> Happy New Year! > >>> > >>> Time to begin release process for the next major release, Django 3.2! > >>> > >>> Reference: https://code.djangoproject.com/wiki/Version3.2Roadmap > >>> > >>> The 3.2 feature freeze is scheduled for January 14. > >>> We'll probably release the alpha that day. > >>> > >>> We have a few larger patches we want to finish reviewing: > >>> > >>> https://github.com/django/django/pull/11929 - Fixed #26167 -- Added > support for functional indexes. > >>> https://github.com/django/django/pull/13618 - Fixed 30360 -- Support > rotation of secret keys. > >>> https://github.com/django/django/pull/13435 - Fixed #32018 -- > Extracted admin colors into CSS variables. > >>> https://github.com/django/django/pull/11026 - Fixed #29010, Fixed > #29138 -- Added limit_choices_to and to_field support to autocomplete > fields. > >>> > >>> We'll update this thread for any schedule changes. > > > > -- > Paolo Melchiorre > > https://www.paulox.net > > -- > You received this message because you are subscribed to the Google Groups > "Django developers (Contributions to Django itself)" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to django-developers+unsubscr...@googlegroups.com. > To view this discussion on the web visit > https://groups.google.com/d/msgid/django-developers/CAKFO%2Bx7sbbZCeJmPGmaf%2BfS8ASCYm-WVxS37nKZAEKFsJ%2BgxXA%40mail.gmail.com > . > -- You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group. To unsubscribe from this group and stop receiving emails from it, send an email to django-developers+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CA%2B6apDqXe19BZhoCLQz8Jc-qYrNphYxZuLzKDf7mP4Py%2BNv53A%40mail.gmail.com.
