bpo-40503: Add documentation and what's new entry for zoneinfo #20006
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
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. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
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 :-)
Misc/NEWS.d/next/Library/2020-05-08-15-48-39.bpo-40503.elZyxc.rst
Outdated
Show resolved
Hide resolved
| use the environment variable ``PYTHONTZPATH``, if it exists, to set the search | ||
| path. | ||
|
|
||
| .. envvar:: PYTHONTZPATH |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 withPYTHON.
Can you open a BPO issue for this?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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).
|
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. |
Co-authored-by: Victor Stinner <vstinner@python.org>
|
OK, @willingc gave me the go-ahead to merge without waiting for her review over e-mail, so we're good to go! 🚀 |
|
Sorry, I didnt understand that the previous implementation PR had no NEWS entry. In that case, your NEWS entry is good 😊 |
|
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. |
…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 <vstinner@python.org>
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.
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.
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