-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
PEP 495 contains incorrect diagram for Mind the Gap section #2959
Comments
Alternatively, maybe use this diagram which more complicated, but is closer to what the code does when the imaginary tz = zoneinfo.ZoneInfo('America/Los_Angeles')
# In the gap, fold=0 selects the first transition extended forward
t = datetime(2022, 3, 13, 2, 29, 0, tzinfo=tz, fold=0)
u0 = t.timestamp()
t0 = datetime.fromtimestamp(u0, tz=tz)
print(t0) # 2022-03-13 03:29:00-07:00
# In the gap, fold=1 selects the second transition extended backward
t = datetime(2022, 3, 13, 2, 29, 0, tzinfo=tz, fold=1)
u1 = t.timestamp()
t1 = datetime.fromtimestamp(u1, tz=tz)
print(t1) # 2022-03-13 01:29:00-08:00 |
Your interpretation would seem to be correct, at least as far as I can tell as a PEP editor. However, as Final PEPs are generally considered historical records rather than living documentation or standards, and this PEP in particular was originally accepted 7 years ago for a Python version released 6 years ago and no longer officially supported, I'm not sure it's worth the effort to update. However, that's ultimately up to the authors @tim-one and @abalkin , though AFAIK neither of them are very active anymore these days. |
I think all Python versions >= 3.6 implement the I understand the reluctance to change stable documents. Can PEP documents be appended with an Errata section? |
Thanks for understanding. There's no real precedent for that, especially for something that is just illustrative background rather than being a normative, substantive part of the specification—either it is fixed if worth fixing, or left alone. If the PEP is to be updated with a new image, then the incorrect image may as well just be replaced directly. In general (with some existing exceptions), once a Standards Track PEP is Final, the canonical documentation or specification for it, if needed, should typically be hosted somewhere more appropriate (such as the Python docs) and updated there. The closest canonical place here AFAIK would be the datetime module docs, where the important parts are documented and the general concepts/background justifying the change (like these cartoons) are not included. But again, with all that said, it's ultimately up to the PEP authors if they think it's worth fixing or not, presuming you'd be willing to submit a suitable replacement (in SVG format, like the originals) in a PR. |
The diagrams that I attached to this ticket are SVG files. If they are satisfactory, I can create a PR to replace https://github.com/python/peps/blob/main/pep-0495-gap.svg with one of those. Either of the 2 above. Oh, I found the original commit that added the incorrect Gap diagram The reason that I am motivated to fix this is because the behavior of the |
This sounds like a bug fix, and I am in favour of fixing bugs. I for one would welcome that PR. |
Here is my reproduction of the correct Gap SVG file that was lost in the original commit. If this is acceptable, I can send a PR to replace the current The reproduction is not perfect:
|
LGTM but I'm not a content expert. |
LGTM, I'm also not a SME, but the background I do have in this area as well as the fact that it matches the original PNG seems to confirm the change is correct. I'd propose you go ahead your PR, and if Tim and Alexander don't object after a week or two, we can merge it.
Ah, I was wondering if that was the case when we moved to the SVG version being displayed relatively recently, which is why it was never noticed at the time, but hadn't actually gone back to check yet. That makes a particularly strong and compelling case for accepting it, since not only is it a bug fix to the PEP itself, but is fixing a technical regression from how the PEP was originally displayed at the time of its acceptance.
If text in the PEP was important to your understanding of the behavior of the |
The SVG image in the "Mind the Gap" section (pep-0495-gap.svg) is incorrect. It is essentially the same as the the SVG image in the "In the Gap" section (pep-0495-fold.svg). This is a bug that dates back to the original commit on 2015-08-30 (814d372) that uploaded the both the PNG and SVG versions of the Fold and Gap images. The PNG version of the Gap image is correct, but the SVG version is incorrect. I believe this bug was invisible prior to 2022-04-23 because PEP 495 previously referenced the PNG versions of the images (pep-0495-gap.png, pep-0495-fold-2.png). Commit 28a5eb3 deleted the PNG images and changed the links in PEP 495 to point to the SVG versions. The replacement SVG image attached in this PR is a manual recreation of the PNG version of the Gap image that was deleted on 2022-04-23. It is not exactly the same, but I think it is close enough.
@CAM-Gerlach : Sent off a PR. With regards to the Python docs, are you referring to https://docs.python.org/3/library/datetime.html ? That is a beast of a page, I'd be reluctant to touch that one. But one obvious improvement might be to actually add a link to the PEP 495 document near the first occurrence of the |
Well, ideally we want the docs to avoid having to reference PEPs to explain things as far as the use of the current implementation goes, as opposed to the historical design decisions that shaped the eventual implementation in CPython. That said, if you really want, it's the https://github.com/python/cpython repo along with the rest of the source code. You can find a link to the source code of every docs page at the top of said page: The link for this particular page is https://github.com/python/cpython/tree/main/Lib/datetime.py |
Thanks. The link at the top of the datetime docs page points to the source code for My opinion is that an average software developer would find it very difficult to understand the An extreme example of such a gap is something like the In any case, let's tackle this one step at a time:
|
Oops, I somehow missed that—my mistake, sorry!
Yup, that would definitely seem to be the case from what you've stated and what you've seen on the page. It gives only very basic API Reference information, and lacks any real explaination-type material discussing the theory and background behind it to actually explain to the reader how it works.
Good plan 👍
👍
It's certainly not any sort of hard rule; PEPs are referenced a number of places to explain the motivation, rationale and history of a specific feature, and it's better to reference the PEP than to simply leave the reader hanging completely when it comes to explanation-type content serving their understanding. What I was suggesting, rather, is that while adding references to the PEP ( Since you were in that position yourself as a reader, your insight as to what parts of the PEP were most useful, and how to best present them in a reader-friendly manner in the docs, would be uniquely valuable here and we'd be happy to help guide you in this. |
I created the gap sketch with Inkscape and the source code was committed to the hg.python.org repo which is still available. I will try to extract the original file an post it here, but from the diff, I see that there was an exporting problem: + inkscape:version="0.91 r13725"
+ sodipodi:docname="pep-0495-gap.svg"
+ inkscape:export-filename="/Users/a/Work/peps/pep-0495-fold.png" |
Yup, that's exactly the same thing as here; the problem was missed in the 7 years since until relatively recently, when the source for the PEP rendering was switched over to the SVG version from the PNG one (since the legacy rendering system we got rid of a year ago didn't support modern SVG). Thanks, let us know! If you can't or its too much trouble, we can always just go ahead with #2965 as is as a backup plan. |
@abalkin : Google Draw renders its subscript significantly below the normal text, see below. The closest I could get to the original was to use 14pt font for the Thanks for the work on PEP 495 by the way! Of all the different ways of handling local time overlaps and gaps that I have surveyed (C++ HowardHinnant/date, C# Noda Time, Java java.time, Java Joda-Time, Go time), PEP 495 is the most reasonable and flexible, especially when implementing a timezone library in a language or environment that does not support exceptions. |
It appears that I only saved latest PNG versions and not the SVG source. I have to admit that it was my first time using Inkscape. Let's go with the versions in the PR for now - it is important to get this fixed. I will try to find time to add a section to the official datetime documentation hopefully with better text and graphics than in the PEP. |
Thanks @abalkin — I'll go ahead and merge that, then 👍 |
In PEP 495, the diagram in the Mind the Gap section is the same as the diagram in the In the Fold section just above it. I believe this is incorrect. The correct diagram should look like the following. I tried to reproduce the font and color scheme, but it's not perfect:
The text was updated successfully, but these errors were encountered: