GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-40503: Add documentation and what's new entry for zoneinfo by pganssle · Pull Request #20006 · python/cpython

Conversation

@pganssle

@pganssle pganssle commented May 8, 2020 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Since #19909 already has over 4000 lines of code in it, I decided to split the documentation PR out from the rest of the PEP 615 implementation to make it easier to review them separately.

For reference, here is the PR with reviews from when the documentation portion of this (not the What's New entry) was added to the reference implementation repo: pganssle/zoneinfo#48

https://bugs.python.org/issue40503

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label May 8, 2020
@bedevere-bot bedevere-bot added the awaiting core review label May 8, 2020
@pganssle
pganssle force-pushed the zoneinfo_docs branch from 744536e to 5618a3b Compare May 8, 2020 20:08
@pganssle pganssle mentioned this pull request May 8, 2020
bpo-40503: Add tests and implementation for ZoneInfo #19909
Merged
@pganssle

pganssle commented May 11, 2020

Copy link
Copy Markdown
Member Author

CC @willingc @merwok Any interest in taking a look?

Thanks!
willingc reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@willingc
willingc self-requested a review May 11, 2020 16:21
@bedevere-bot

This comment has been minimized.

Sign in to view
@bedevere-bot

This comment has been minimized.

Sign in to view
@bedevere-bot

This comment has been minimized.

Sign in to view
@bedevere-bot

This comment has been minimized.

Sign in to view
@bedevere-bot

This comment has been minimized.

Sign in to view
@bedevere-bot

This comment has been minimized.

Sign in to view
@pganssle

pganssle commented May 11, 2020

Copy link
Copy Markdown
Member Author

Sorry for the spam everyone, the buildbots seem convinced that this PR is #19909 I guess and are putting a bunch of refleak failure notifications as comments here.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot

This comment has been minimized.

Sign in to view
@bedevere-bot

This comment has been minimized.

Sign in to view
@bedevere-bot

This comment has been minimized.

Sign in to view
@bedevere-bot

This comment has been minimized.

Sign in to view
@bedevere-bot

This comment has been minimized.

Sign in to view
pablogsal added a commit to pablogsal/buildmaster-config that referenced this pull request May 11, 2020
@pablogsal
Do not report back to GitHub with the PR buildeers
27d3d9c 
It seems that the PR builders can get confused if a pull requests
contains commits of another pull request when ussing the
`test-with-buildbots` label. For instance, this PR:

python/cpython#20006

included messages that report to a commit that lives in another PR where
the label was added. To avoid this, exclude the PR builders from the
GitHub notification system.
@pablogsal pablogsal mentioned this pull request May 11, 2020
Do not report back to GitHub with the PR buildeers python/buildmaster-config#193
Merged
@bedevere-bot

This comment has been minimized.

Sign in to view
@pganssle
pganssle force-pushed the zoneinfo_docs branch from 5618a3b to 83fc392 Compare May 16, 2020 14:58
vstinner
vstinner approved these changes May 16, 2020
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I suggest to go ahead with the PR unmodified (to ship beta1 with a documentation), but then write a following PR to address my review.

Maybe just move the NEWS entry to the Documentation, or even remove it. It says that it adds the zoneinfo module, but the module was already added. Either rephase it to say that you adds zoneinfo doc, or simply skip the NEWS entry. I don't think that it's worth it to add a NEWS entry.

I wanted to play with zoneinfo, but I failed to find its documentation, then I found this PR :-)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Doc/whatsnew/3.9.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Misc/NEWS.d/next/Library/2020-05-08-15-48-39.bpo-40503.elZyxc.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Doc/library/zoneinfo.rst Outdated

@vstinner vstinner May 16, 2020

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may move this documentation to Doc/using/cmdline.rst, and keep a minimum documention here using the following syntax here to get a link.

:envvar:`PYTHONTZPATH`

By the way, the zoneinfo module should ignore PYTHONTZPATH is sys.flags.ignore_environment is true. See for example Lib/importlib/_bootstrap_external.py, since the env var name starts with PYTHON.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@pganssle pganssle May 16, 2020

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, the zoneinfo module should ignore PYTHONTZPATH is sys.flags.ignore_environment is true. See for example Lib/importlib/_bootstrap_external.py, since the env var name starts with PYTHON.

Can you open a BPO issue for this?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@pganssle pganssle May 16, 2020

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may move this documentation to Doc/using/cmdline.rst, and keep a minimum documention here using the following syntax here to get a link.

This is a good idea, but I'd like to save it for a later PR because I don't think I have the time right now to really evaluate and test that change (which is mostly cosmetic / organizational).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vstinner vstinner May 16, 2020

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I wrote in my approval message, I suggest you to make the PR as it is. My minor remarks can be addressed later (maybe just fix the NEWS entry).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels May 16, 2020
@vstinner

vstinner commented May 16, 2020

Copy link
Copy Markdown
Member

Note: I'm happy to see the that new GitHub Action workflow configuration works as expected: "build" jobs are skipped since this PR only changes the documentation, not the code.
pganssle reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
pganssle added 4 commits May 16, 2020 11:51
@pganssle
Add documentation for zoneinfo module
8b10cb3
@pganssle
Add What's New entry for zoneinfo
77e0396
@pganssle
Add news entry
f90eb0c
@pganssle
Remove unused label
ea35e5c
pganssle and others added 2 commits May 16, 2020 11:51
@pganssle
Add documentation for TZPATH compile option
02b05da
@pganssle @vstinner
Update Misc/NEWS.d/next/Library/2020-05-08-15-48-39.bpo-40503.elZyxc.rst
78576ea 
Co-authored-by: Victor Stinner <[email protected]>
@pganssle
pganssle force-pushed the zoneinfo_docs branch from 726914d to 78576ea Compare May 16, 2020 15:51
pganssle
pganssle commented May 16, 2020
View reviewed changes
Comment thread Doc/library/zoneinfo.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
@pganssle

pganssle commented May 16, 2020

Copy link
Copy Markdown
Member Author

OK, @willingc gave me the go-ahead to merge without waiting for her review over e-mail, so we're good to go! 🚀 :shipit:
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@pganssle
pganssle merged commit b17e49e into python:master May 16, 2020
@bedevere-bot bedevere-bot removed the awaiting merge label May 16, 2020
@vstinner

vstinner commented May 16, 2020

Copy link
Copy Markdown
Member

Sorry, I didnt understand that the previous implementation PR had no NEWS entry. In that case, your NEWS entry is good 😊
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner

vstinner commented May 18, 2020

Copy link
Copy Markdown
Member

Aaaaand, it's now online: https://docs.python.org/dev/library/zoneinfo.html ! For me, it's easier to review the nice HTML output than raw .rst file.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
arturoescaip pushed a commit to arturoescaip/cpython that referenced this pull request May 24, 2020
@pganssle @vstinner @arturoescaip
bpo-40503: Add documentation and what's new entry for zoneinfo (pytho…
8fe1f73 
…nGH-20006)

This adds the documentation for the `zoneinfo` module added in PEP 615: https://www.python.org/dev/peps/pep-0615/

The implementation itself was pythonGH-19909: python#19909

bpo-40503: https://bugs.python.org/issue40503

Co-authored-by: Victor Stinner <[email protected]>
pablogsal added a commit to python/buildmaster-config that referenced this pull request Jan 26, 2021
@pablogsal
Do not report back to GitHub with the PR buildeers (#193)
75b2158 
It seems that the PR builders can get confused if a pull requests
contains commits of another pull request when ussing the
`test-with-buildbots` label. For instance, this PR:

python/cpython#20006

included messages that report to a commit that lives in another PR where
the label was added. To avoid this, exclude the PR builders from the
GitHub notification system.
@pganssle pganssle mentioned this pull request Apr 10, 2022
PEP 615: Add zoneinfo module #84683
Closed
JokoZulkarnain99 added a commit to JokoZulkarnain99/buildmaster-config that referenced this pull request Aug 5, 2024
@JokoZulkarnain99
Do not report back to GitHub with the PR buildeers (#193)
9e18a7f 
It seems that the PR builders can get confused if a pull requests
contains commits of another pull request when ussing the
`test-with-buildbots` label. For instance, this PR:

python/cpython#20006

included messages that report to a commit that lives in another PR where
the label was added. To avoid this, exclude the PR builders from the
GitHub notification system.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner approved these changes
@willingc willingc Awaiting requested review from willingc

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@pganssle @bedevere-bot @vstinner @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.