bpo-30699: Improve example on tzinfo instances #4290

mariocj89 · 2017-11-05T19:57:16Z

Move from GMTX to TZX when naming the classes, as GMT1 might be rather confusing as seen in the reported issue.
In addition, move to UTC over GMT and improve the tzname implementation.

https://bugs.python.org/issue30699

vstinner

LGTM.

Mariatta · 2018-03-01T18:55:50Z

Appveyor was stuck. I'll close and re-open the PR to retrigger it.

Mariatta · 2018-03-01T18:57:46Z

Thanks, the changes LGTM. If no objection from @abalkin I'll merge it.

mariocj89 · 2018-03-01T19:09:37Z

Thanks a lot for reviewing this old PRs :) really appreciated

abalkin · 2018-03-01T19:41:28Z

If we are going to update these examples, I think we should make them PEP 495 compliant and I don't think this PR achieves it.

mariocj89 · 2018-03-01T21:52:27Z

@abalkin not sure I understand what you mean. The examples are moved form UTC naming to GMT, there is no geographical timezone involved and therefore the fold attribute is not relevant. Isn't it?

abalkin · 2018-03-02T17:49:08Z

@mariocj89 - the fold attribute is only irrelevant for a TZ with a never changing offset. As soon as you have DST changes as is the case in these examples, you have to take fold into account when implementing the .utcoffset() and .dst() methods and the .fromutc() method should properly set the fold attribute. See Guidelines for New tzinfo Implementations.

mariocj89 · 2018-03-02T19:56:35Z

@abalkin absolutely right, missed the UTC +2" if self.dst(dt) else "UTC +1 in the example. I'll have a look into updating the PR to include that

abalkin

Let's make the examples PEP 495 compliant.

bedevere-bot · 2018-03-02T21:28:21Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be poked with soft cushions!

pganssle · 2018-03-17T15:34:18Z

@abalkin I trust Mario to come up with something here, but I do think writing a fold-aware example implementation would be way easier if this issue were resolved. I think that would also make fromutc implementations that wrap tzinfo.fromutc much faster, since most of the code would be executed in C (though I could be wrong on that point).

I think the fact that it's actually fairly difficult to write a fromutc that's simple enough for a documentation example might be a sign that we should try to alleviate that cognitive overhead by having basic fold support in tzinfo.fromutc.

mariocj89 · 2018-03-17T15:37:22Z

I am currently quite overwhelmed with work and other organisations, but as soon as I have time i’d Like to get into it and update the examples. I realised when doing it that it is not a 10 minutes job :p

TL;DR
I haven’t forgotten, will get into it “soon”

mariocj89 · 2018-06-03T16:50:11Z

@abalkin The code for the timezone classes with support for dst and fold attributes is rather complex.

Given the section is to illustrate how to work with datetime and tzinfo objects, what about moving to timezone.utc and a user-defined timezone class based on Kabul? I chose Kabul as it moved its offset once but has no ambiguous times (only imaginary).

This still showcases how datetime and tzinfo interact and the code is much simpler!

Note there are examples on how to work with tzinfo further down the file.

Example implementation: mariocj89@c132630

I can push this to this PR for review if the idea looks good.

abalkin · 2018-06-04T17:22:15Z

@mariocj89 - Using Asia/Kabul for the documentation example sounds fine, but your implementation is incomplete. For the "imaginary" times the result of utcoffset() must depend on the fold value. See Mind the Gap section in PEP 495.

mariocj89 · 2018-06-04T18:15:15Z

Good point! Always thought that the PEP referred only to DST but indeed apply to all “variable offsets”.

Will then update it and push it to this branch. Thanks.

pganssle · 2018-06-05T12:38:44Z

As a note, if something like #7425 (which adds fold support directly into the default fromutc implementation) were to be merged, you could write a super-basic fold-aware implementation like this:

from datetime import datetime, timedelta, tzinfo, timezone

UTC = timezone.utc

class ExampleFold(tzinfo):
    def tzname(self, dt):
        if self.dst(dt):
            return "DST"
        else:
            return "STD"

    def utcoffset(self, dt):
        return timedelta(hours=0) + self.dst(dt)

    def dst(self, dt):
        dt_start = datetime(1, 3, 17, 1).replace(year=dt.year)
        dt_end = datetime(1, 11, 4, 1).replace(year=dt.year)

        naive_dt = dt.replace(tzinfo=None)

        dst_offset = timedelta(hours=1)
        if dt_start <= naive_dt < dt_end:
            return dst_offset
        else:
            if naive_dt < dt_end + dt_offset:
                if dt.fold:
                    return timedelta(0)
                else:
                    return dst_offset

            return timedelta(0)

Obviously that's a sorta weird case because there's no timezone that uses fixed days of the year for offsets, but you can see how easy the "add fold support" part is once fromutc respects it.

mariocj89 · 2018-06-05T15:58:38Z

@abalkin I've pushed a potential example implementation. Had to override fromutc as well, maybe there is a simpler way :/ Let me know if anything needs changing.

Also updated the content to pass doctests (only the modified part).

Tested that Kabul implementation against the dateutil one before at and after the move.

Doc/library/datetime.rst

vstinner · 2019-01-10T16:02:20Z

I removed the " needs backport to 3.6" label, the 3.6 branch no longer accept bugfixes (only security fixes are accepted): https://devguide.python.org/#status-of-python-branches

pganssle

Almost all of my comments here are completely optional and I think this can be merged whether they are incorporated or not.

The only "hard" change that I recommend is the use of "+01:00" instead of "UTC+1" in the TZ1 class.

pganssle · 2019-01-12T16:56:03Z

Doc/library/datetime.rst

+    datetime.timedelta(0, 14400)
+    >>> # Datetime after the change
+    >>> dt2 = datetime(2006, 6, 14, 13, 0, tzinfo=tz1)
    >>> dt2.utcoffset()


Can we change this to:

>>> print(dt.utcoffset()) 4:30:00

It's a very minor thing, but to me the standard repr for timedelta is completely unreadable.

pganssle · 2019-01-12T16:57:59Z

Doc/library/datetime.rst

-    ...             return timedelta(0)
-    ...     def tzname(self,dt):
-    ...         return "GMT +2"
+    ...             return "+04"


I would mildly prefer "+04:00" here for consistency, but I don't care strongly about this, it's just a documentation example and I do realize that @abalkin suggested this label.

Doc/library/datetime.rst

pganssle · 2019-01-12T17:08:44Z

Doc/library/datetime.rst

    ...         return timedelta(0)
    ...     def tzname(self,dt):
-    ...         return "Europe/Prague"
+    ...         return "UTC+1"


Super annoyingly, POSIX-style offset names have an inverted sense, so dateutil.parser.parse (and I think TZ variables) would interpret UTC+1 as the same as -01:00. Of course, many people use this in the opposite sense, which gives rise to some confusion.

I recommend bypassing the whole thing by returning "+01:00" here.

Doc/library/datetime.rst

Move from GMTX to TZX when naming the classes, as GMT1 might be rather confusing as seen in the reported issue. In addition, move to UTC over GMT and improve the tzname implementation.

Move the example in the documentation to just use timezone.utc and a user defined Kabul timezone rather than having two user defined timezones with DST. Kabul timezone is still interesting as it changes its offset but not based on DST. This is more accurate as the previous example was missing information about the fold attribute. Additionally, implementing the fold attribute was rather complex and probably not relevant enough for the section "datetime with tzinfo".

csabella · 2019-06-01T01:23:26Z

@mariocj89 @pganssle
There was a lot of activity on this issue and 2 approvals, but I'm not quite sure where it's at. Can you take another look and comment if it's ready to merge? Thanks!

mariocj89 · 2019-06-01T10:26:25Z

Wow! Totally forgot about this PR 😆 .

@abalkin requested to make this PEP 495 complient.

There were some suggestions by @pganssle as well but were the opposite of what @abalkin was proposing when formatting offsets.

Let me know if there is any change to make!

pganssle · 2019-06-01T13:17:06Z

We can ignore any of my comments that contradict @abalkin's. This is clearly an improvement over the previous state of affairs, and we can always change it later.

vstinner · 2019-06-04T14:24:45Z

@pganssle: You didn't approve this PR. Does it mean that it still lacks someone, or is it good as it is? If yes, can you please officially approve it?

pganssle

@vstinner No problem, approved. I think this change is clearly an improvement over the status quo.

I think documentation backports are only going back to 3.7 now anyway, but in case they aren't, this relies on PEP 495, so it can't be backported any further than 3.6. I think it's fine to backport to 3.7 unless @mariocj89 disagrees.

@abalkin

Paul wrote: "We can ignore any of my comments that contradict @abalkin's."

miss-islington · 2019-06-04T15:18:15Z

Thanks @mariocj89 for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

* Improve example on tzinfo instances Move from GMTX to TZX when naming the classes, as GMT1 might be rather confusing as seen in the reported issue. In addition, move to UTC over GMT and improve the tzname implementation. * Simplify datetime with tzinfo example Move the example in the documentation to just use timezone.utc and a user defined Kabul timezone rather than having two user defined timezones with DST. Kabul timezone is still interesting as it changes its offset but not based on DST. This is more accurate as the previous example was missing information about the fold attribute. Additionally, implementing the fold attribute was rather complex and probably not relevant enough for the section "datetime with tzinfo". (cherry picked from commit f0b5ae4) Co-authored-by: Mario Corchero <mcorcherojim@bloomberg.net>

vstinner · 2019-06-04T15:18:39Z

I think documentation backports are only going back to 3.7 now anyway, but in case they aren't, this relies on PEP 495, so it can't be backported any further than 3.6. I think it's fine to backport to 3.7 unless @mariocj89 disagrees.

Python 3.6 no longer accept doc changes, only security fixes: https://devguide.python.org/#status-of-python-branches

bedevere-bot · 2019-06-04T15:18:43Z

GH-13811 is a backport of this pull request to the 3.7 branch.

* Improve example on tzinfo instances Move from GMTX to TZX when naming the classes, as GMT1 might be rather confusing as seen in the reported issue. In addition, move to UTC over GMT and improve the tzname implementation. * Simplify datetime with tzinfo example Move the example in the documentation to just use timezone.utc and a user defined Kabul timezone rather than having two user defined timezones with DST. Kabul timezone is still interesting as it changes its offset but not based on DST. This is more accurate as the previous example was missing information about the fold attribute. Additionally, implementing the fold attribute was rather complex and probably not relevant enough for the section "datetime with tzinfo". (cherry picked from commit f0b5ae4) Co-authored-by: Mario Corchero <mcorcherojim@bloomberg.net>

vstinner · 2019-06-04T15:29:49Z

Thanks Mario Corchero for the doc enhancement, and Paul for the review!

* Improve example on tzinfo instances Move from GMTX to TZX when naming the classes, as GMT1 might be rather confusing as seen in the reported issue. In addition, move to UTC over GMT and improve the tzname implementation. * Simplify datetime with tzinfo example Move the example in the documentation to just use timezone.utc and a user defined Kabul timezone rather than having two user defined timezones with DST. Kabul timezone is still interesting as it changes its offset but not based on DST. This is more accurate as the previous example was missing information about the fold attribute. Additionally, implementing the fold attribute was rather complex and probably not relevant enough for the section "datetime with tzinfo".

the-knights-who-say-ni added the CLA signed label Nov 5, 2017

bedevere-bot added the awaiting review label Nov 5, 2017

mariocj89 force-pushed the gmtdocs branch from ffc3feb to 7db9cba Compare November 5, 2017 20:01

vstinner approved these changes Nov 6, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Nov 6, 2017

vstinner requested a review from abalkin November 6, 2017 15:38

vstinner added the skip news label Nov 6, 2017

Mariatta closed this Mar 1, 2018

Mariatta reopened this Mar 1, 2018

Mariatta approved these changes Mar 1, 2018

View reviewed changes

Mariatta added needs backport to 3.6 labels Mar 1, 2018

abalkin previously requested changes Mar 2, 2018

View reviewed changes

bedevere-bot removed the awaiting merge label Mar 2, 2018

bedevere-bot added the awaiting changes label Mar 2, 2018

abalkin reviewed Jun 5, 2018

View reviewed changes

Doc/library/datetime.rst Outdated Show resolved Hide resolved

serhiy-storchaka added the docs Documentation in the Doc dir label Dec 8, 2018

vstinner removed the needs backport to 3.6 label Jan 10, 2019

pganssle requested changes Jan 12, 2019

View reviewed changes

mariocj89 added 2 commits February 2, 2019 12:08

Improve example on tzinfo instances

8137fbf

Move from GMTX to TZX when naming the classes, as GMT1 might be rather confusing as seen in the reported issue. In addition, move to UTC over GMT and improve the tzname implementation.

mariocj89 force-pushed the gmtdocs branch from 56e7adf to 3110dc3 Compare February 2, 2019 17:38

csabella requested a review from abalkin June 1, 2019 01:23

pganssle approved these changes Jun 4, 2019

View reviewed changes

vstinner merged commit f0b5ae4 into python:master Jun 4, 2019

bedevere-bot removed the awaiting changes label Jun 4, 2019

bedevere-bot removed the needs backport to 3.7 label Jun 4, 2019

Uh oh!

bpo-30699: Improve example on tzinfo instances #4290

bpo-30699: Improve example on tzinfo instances #4290

Uh oh!

Conversation

mariocj89 commented Nov 5, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Mariatta commented Mar 1, 2018

Uh oh!

Mariatta commented Mar 1, 2018

Uh oh!

mariocj89 commented Mar 1, 2018

Uh oh!

abalkin commented Mar 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mariocj89 commented Mar 1, 2018

Uh oh!

abalkin commented Mar 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mariocj89 commented Mar 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abalkin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Mar 2, 2018

Uh oh!

pganssle commented Mar 17, 2018

Uh oh!

mariocj89 commented Mar 17, 2018

Uh oh!

mariocj89 commented Jun 3, 2018

Uh oh!

abalkin commented Jun 4, 2018

Uh oh!

mariocj89 commented Jun 4, 2018

Uh oh!

pganssle commented Jun 5, 2018

Uh oh!

mariocj89 commented Jun 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

vstinner commented Jan 10, 2019

Uh oh!

pganssle left a comment

Choose a reason for hiding this comment

Uh oh!

pganssle Jan 12, 2019

Choose a reason for hiding this comment

Uh oh!

pganssle Jan 12, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pganssle Jan 12, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

csabella commented Jun 1, 2019

Uh oh!

mariocj89 commented Jun 1, 2019

Uh oh!

pganssle commented Jun 1, 2019

Uh oh!

vstinner commented Jun 4, 2019

Uh oh!

pganssle left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miss-islington commented Jun 4, 2019

Uh oh!

mariocj89 commented Nov 5, 2017 •

edited by bedevere-bot

Loading

abalkin commented Mar 1, 2018 •

edited

Loading

abalkin commented Mar 2, 2018 •

edited

Loading

mariocj89 commented Mar 2, 2018 •

edited

Loading

abalkin left a comment •

edited

Loading

mariocj89 commented Jun 5, 2018 •

edited

Loading

pganssle left a comment •

edited

Loading