bpo-40503: Add tests and implementation for ZoneInfo#19909
Conversation
Sorry, something went wrong.
|
@python/windows-team I can't really make heads or tails of the azure pipelines configuration, but I need to try to add a PyPI dependency to the tests (it's particularly important on Windows). Can one of y'all point me in the right direction here? |
Sorry, something went wrong.
pganssle
added
the
🔨 test-with-buildbots
Test PR w/ buildbots; report in status section
label
May 11, 2020
bedevere-bot
removed
the
🔨 test-with-buildbots
Test PR w/ buildbots; report in status section
label
May 11, 2020
|
@serhiy-storchaka Would you mind taking a look at the C code here? There's about 2000 lines of it and it would be good to get an expert eye on it. Also CC-ing @vstinner because it's getting close to feature freeze and there's a ton of code to review so I'm now trying to dragoon specific people instead of sending out general requests for volunteers 😛 |
Sorry, something went wrong.
|
Rebasing this to fix merge conflicts, and I'm going to squash down many of the fixup commits as part of that. Hopefully it doesn't hurt peoples' ability to give this an incremental review too badly. |
Sorry, something went wrong.
|
@pganssle @ambv: I would prefer to get this PR merged ASAP and not wait the last minute (like monday evening). It's very common that when such large feature is merged, many issues pops up and block a release. It seems like the the CI configuration changes (pip install -r Misc/test-requirements.txt) is kind of controversial. I discussed in private with Paul and he told me that not all test_zoneinfo tests are skipped if tzdata is missing. I suggest to remove Misc/test-requirements.txt and all related CI changes for now, and then propose a follow-up PR which the same changes (or a subset, it's up to you). The idea is just to unblock the PR to be able to merge ASAP. I also expect some bad surprised with "pip install tzdata" on CIs, and so I would prefer to wait after beta1 dust fall, to have more time to investigate these issues. Otherwise, the feature will land late, cause issues, and delay Python 3.9 beta1 release. I'm not going to approve the PR with the current CI configuration changes, because @zooba asked for changes, but also because I'm not sure that it's a best approach (and I don't have time to investigate the issue to see what are the different options). PyPI dependencies is something new and we should take our time to agree on the best solution. https://discuss.python.org/t/optional-pypi-dependencies-in-ci-jobs/4111 I don't have the bandwidth to review such large PR. @pganssle asked for review on python-dev, Twitter and other ways. Well, there are few people available and few people who understands time zones to review properly the code. I'm fine with merging the code as it and enhance the code later (if the CI config changes are reverted :-)). Well, basically do a "post-commit review" :-) It's just that everybody it busy with other stuff just before the feature freeze. I would like to have one Python 3.9 release on the announced date ;-) Especially such important milestone (feature freeze)! I would strongly prefer to not merge zoneinfo after the beta1. |
Sorry, something went wrong.
|
Per Victor's suggestion, I plan to merge this today, so if anyone has been putting off reviewing this until the last minute, please do so now. As mentioned earlier, I would prefer it if we can keep |
Sorry, something went wrong.
|
Per a sidebar discussion with @zooba, for now we'll compromise on leaving the I will come up with a more thorough proposal for how to do those tests later, but as long as it's hit during the Python coverage job I'm more comfortable leaving it out of the main tests. I believe this is ready to merge now, I'll give it a few hours for any last minute reviews, but anyone can feel free to make inline comments even after the merge. |
Sorry, something went wrong.
This is the initial implementation of PEP 615, the zoneinfo module, ported from the standalone reference implementation (see https://www.python.org/dev/peps/pep-0615/#reference-implementation for a link, which has a more detailed commit history). This includes (hopefully) all functional elements described in the PEP, but documentation is found in a separate PR. This includes: 1. A pure python implementation of the ZoneInfo class 2. A C accelerated implementation of the ZoneInfo class 3. Tests with 100% branch coverage for the Python code (though C code coverage is less than 100%). 4. A compile-time configuration option on Linux (though not on Windows) Differences from the reference implementation: - The module is arranged slightly differently: the accelerated module is `_zoneinfo` rather than `zoneinfo._czoneinfo`, which also necessitates some changes in the test support function. (Suggested by Victor Stinner and Steve Dower.) - The tests are arranged slightly differently and do not include the property tests. The tests live at test/test_zoneinfo/test_zoneinfo.py rather than test/test_zoneinfo.py or test/test_zoneinfo/__init__.py because we may do some refactoring in the future that would likely require this separation anyway; we may: - include the property tests - automatically run all the tests against both pure Python and C, rather than manually constructing C and Python test classes (similar to the way this works with test_datetime.py, which generates C and Python test cases from datetimetester.py). - This includes a compile-time configuration option on Linux (though not on Windows); added with much help from Thomas Wouters. - Integration into the CPython build system is obviously different from building a standalone zoneinfo module wheel. - This includes configuration to install the tzdata package as part of CI, though only on the coverage jobs. Introducing a PyPI dependency as part of the CI build was controversial, and this is seen as less of a major change, since the coverage jobs already depend on pip and PyPI. Additional changes that were introduced as part of this PR, most / all of which were backported to the reference implementation: - Fixed reference and memory leaks With much debugging help from Pablo Galindo - Added smoke tests ensuring that the C and Python modules are built The import machinery can be somewhat fragile, and the "seamlessly falls back to pure Python" nature of this module makes it so that a problem building the C extension or a failure to import the pure Python version might easily go unnoticed. - Adjustments to zoneinfo.__dir__ Suggested by Petr Viktorin. - Slight refactorings as suggested by Steve Dower. - Removed unnecessary if check on std_abbr Discovered this because of a missing line in branch coverage.
pganssle
added
the
🔨 test-with-buildbots
Test PR w/ buildbots; report in status section
label
May 15, 2020
bedevere-bot
removed
the
🔨 test-with-buildbots
Test PR w/ buildbots; report in status section
label
May 15, 2020
|
I have squashed the commits manually so that I could write a thorough commit message. If anyone wants to see the pre-squashed version for whatever reason, I've preserved it on this branch (plus the reference implementation has a much more detailed commit history anyway): https://github.com/pganssle/cpython/tree/zoneinfo_pre_squash I have kicked off a final run of the buildbots, and unless there are any objections, I will merge it when everything is green. |
Sorry, something went wrong.
|
I merged your PR, thanks Paul. The PEP is great addition and long awaited feature! We still have some months to refine the implementation and decide how to install tzdata when running tests. It is good to have it the first beta1, so users can start to play with it. Should the Windows installer install tzdata? Or should something explains how to install it if it is missing? Impressive: we have 72 CI! All are green. Even Refleaks buildbots! |
Sorry, something went wrong.
) This adds the documentation for the `zoneinfo` module added in PEP 615: https://www.python.org/dev/peps/pep-0615/ The implementation itself was GH-19909: #19909 bpo-40503: https://bugs.python.org/issue40503 Co-authored-by: Victor Stinner <vstinner@python.org>
…9909) This is the initial implementation of PEP 615, the zoneinfo module, ported from the standalone reference implementation (see https://www.python.org/dev/peps/pep-0615/#reference-implementation for a link, which has a more detailed commit history). This includes (hopefully) all functional elements described in the PEP, but documentation is found in a separate PR. This includes: 1. A pure python implementation of the ZoneInfo class 2. A C accelerated implementation of the ZoneInfo class 3. Tests with 100% branch coverage for the Python code (though C code coverage is less than 100%). 4. A compile-time configuration option on Linux (though not on Windows) Differences from the reference implementation: - The module is arranged slightly differently: the accelerated module is `_zoneinfo` rather than `zoneinfo._czoneinfo`, which also necessitates some changes in the test support function. (Suggested by Victor Stinner and Steve Dower.) - The tests are arranged slightly differently and do not include the property tests. The tests live at test/test_zoneinfo/test_zoneinfo.py rather than test/test_zoneinfo.py or test/test_zoneinfo/__init__.py because we may do some refactoring in the future that would likely require this separation anyway; we may: - include the property tests - automatically run all the tests against both pure Python and C, rather than manually constructing C and Python test classes (similar to the way this works with test_datetime.py, which generates C and Python test cases from datetimetester.py). - This includes a compile-time configuration option on Linux (though not on Windows); added with much help from Thomas Wouters. - Integration into the CPython build system is obviously different from building a standalone zoneinfo module wheel. - This includes configuration to install the tzdata package as part of CI, though only on the coverage jobs. Introducing a PyPI dependency as part of the CI build was controversial, and this is seen as less of a major change, since the coverage jobs already depend on pip and PyPI. Additional changes that were introduced as part of this PR, most / all of which were backported to the reference implementation: - Fixed reference and memory leaks With much debugging help from Pablo Galindo - Added smoke tests ensuring that the C and Python modules are built The import machinery can be somewhat fragile, and the "seamlessly falls back to pure Python" nature of this module makes it so that a problem building the C extension or a failure to import the pure Python version might easily go unnoticed. - Adjustments to zoneinfo.__dir__ Suggested by Petr Viktorin. - Slight refactorings as suggested by Steve Dower. - Removed unnecessary if check on std_abbr Discovered this because of a missing line in branch coverage.
…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>
Reviewers
8 participants
This PR brings in the tests, C and pure Python implementations for the
zoneinfomodule, as specified in PEP 615. Migrating this over from the reference implementation.For the sake of keeping the review somewhat tightly scoped (HAH) it does not cover:
The compile-time configuration of(Moved to this PR)TZPATH(bpo-40503: Add compile-time configuration to PEP 615 #20034)Those will come in later / separate PRs.
https://bugs.python.org/issue40503