bpo-36004: Add date.fromisocalendar by pganssle · Pull Request #11888 · python/cpython · GitHub

This commit implements the first version of date.fromisocalendar, the inverse function for date.isocalendar. It is currently missing error checking for the case of of invalid ISO dates in week 53.

To Do:

Validate ISO datetimes
Test for TypeError
Add documentation
Add news entry

Other than these known errors, the existing code can be reviewed. Ready to go.

bpo-36004

https://bugs.python.org/issue36004

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.

One thing I'll note is that this is consistent with datetime.date itself not accepting floats. I'm not sure how much I agree with that, I just wanted to put it in the tests to ensure consistency between the C and Python versions.

I have made the requested changes; please review again.

Thanks for making the requested changes!

@pablogsal: please review the changes made to this pull request.

I think we are missing an entry in Doc/whatsnew/3.8.rst

@pablogsal Indeed, I'm so used to adding these after the fact, it might be nice to actually have one merged with the change for once. Added.

Dismissed my review while I have to do another one to not block the PR

Maybe use MINYEAR/MAXYEAR here.

Instead of converting these over, I have just added a MINYEAR and MAXYEAR test. Even though it's redundant, I like to have the explicit callout of the current boundaries, so that if MINYEAR or MAXYEAR get modified in a way that breaks backwards compatibility, it will raise an error.

Would it be possible to use 3 loops to test all combinations, rather than generate these combinations manualy?

OK, I've converted it over. I think it's a little harder to understand what's going on in the version where the test cases are generated (compared to manual), but using a loop also has its advantages.

I forgot to say that more generally, I like the idea. I like the proposed new constructor, it perfectly makes sense. I just have some comments on the actual implementation.

This commit implements the first version of date.fromisocalendar, the inverse function for date.isocalendar. It is currently missing error checking for the case of of invalid iso dates in week 53. bpo-36004: https://bugs.python.org/issue36004

This avoids an overflow error in ordinal calculations in the C implementation.

This is equivalent but uses only existing helper functions and in many cases will be slightly more efficient.

LGTM.

Side note: @pganssle: maybe it would be interesting to convert datetime to Argument Clinic to provide better docstrings.

Azure Pipelines PR — #20190429.29 failed

Random network issue, unrelated to this change.

@vstinner Thanks for the review and merge, Victor!

* Clarify impact on default behaviour of exec, eval, etc * Update documentation for changes to PyEval_GetLocals (pythongh-74929) Closes pythongh-11888

* Clarify impact on default behaviour of exec, eval, etc * Update documentation for changes to PyEval_GetLocals (gh-74929) Closes gh-11888

* Clarify impact on default behaviour of exec, eval, etc * Update documentation for changes to PyEval_GetLocals (pythongh-74929) Closes pythongh-11888 (cherry picked from commit 2180991) Co-authored-by: Alyssa Coghlan <ncoghlan@gmail.com>

* Clarify impact on default behaviour of exec, eval, etc * Update documentation for changes to PyEval_GetLocals (pythongh-74929) Closes pythongh-11888

the-knights-who-say-ni added the CLA signed label Feb 16, 2019

bedevere-bot added the awaiting review label Feb 16, 2019

pablogsal previously requested changes Feb 17, 2019

View reviewed changes

Comment thread Modules/_datetimemodule.c Outdated

bedevere-bot added awaiting changes and removed awaiting review labels Feb 17, 2019

pganssle commented Feb 17, 2019

View reviewed changes

Comment thread Lib/datetime.py Outdated

ZackerySpytz reviewed Feb 17, 2019

View reviewed changes

Comment thread Modules/_datetimemodule.c Outdated

Comment thread Modules/_datetimemodule.c Outdated

pganssle force-pushed the add_fromisoformat branch 3 times, most recently from 221f2a7 to 3d3e6e7 Compare February 17, 2019 17:38

pganssle changed the title bpo-36004: Add date.fromisocalendar and tests bpo-36004: Add date.fromisocalendar Feb 17, 2019

pganssle commented Feb 17, 2019

View reviewed changes

bedevere-bot added awaiting change review and removed awaiting changes labels Feb 18, 2019

pablogsal reviewed Mar 16, 2019

View reviewed changes

pganssle force-pushed the add_fromisoformat branch from ba63b21 to 44f9adf Compare March 17, 2019 19:18

pganssle force-pushed the add_fromisoformat branch from 44f9adf to 889f58c Compare March 17, 2019 19:44

pganssle force-pushed the add_fromisoformat branch from 889f58c to a9ba64d Compare April 3, 2019 19:05

pganssle force-pushed the add_fromisoformat branch from a9ba64d to c514f03 Compare April 13, 2019 00:14

pganssle force-pushed the add_fromisoformat branch from c514f03 to 5f40dd5 Compare April 20, 2019 18:31

pganssle force-pushed the add_fromisoformat branch from 5f40dd5 to 535a713 Compare April 27, 2019 16:54

vstinner reviewed Apr 28, 2019

View reviewed changes

pganssle added 2 commits April 29, 2019 07:43

Add date.fromisocalendar and tests

6d323ad

This commit implements the first version of date.fromisocalendar, the inverse function for date.isocalendar. It is currently missing error checking for the case of of invalid iso dates in week 53. bpo-36004: https://bugs.python.org/issue36004

Add early year check to fromisocalendar

fbab728

This avoids an overflow error in ordinal calculations in the C implementation.

pganssle added 11 commits April 29, 2019 07:43

Add bounds checking for ISO long vs. short weeks

3ecf02e

Add more comments with test cases

e645cd0

Switch to new method of detecting ISO long years

86edee5

This is equivalent but uses only existing helper functions and in many cases will be slightly more efficient.

Fix missing argument in PyErr_Format

74fb523

Add test for TypeError handling

46c6458

Add documentation for date(time).fromisocalendar

28c7fb2

Add NEWS entry

99e75d7

Add What's New entry for fromisocalendar

c269a5c

Use MINYEAR and MAXYEAR in bounds checks

c8444b7

Add tests for MINYEAR and MAXYEAR

958ffff

Use generated test cases instead of manual list

3307865

pganssle force-pushed the add_fromisoformat branch from 535a713 to 3307865 Compare April 29, 2019 12:00

pganssle closed this Apr 29, 2019

pganssle reopened this Apr 29, 2019

vstinner approved these changes Apr 29, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Apr 29, 2019

vstinner merged commit 88c0937 into python:master Apr 29, 2019

bedevere-bot removed the awaiting merge label Apr 29, 2019

ncoghlan added a commit to ncoghlan/cpython that referenced this pull request Jun 1, 2024

pythongh-11888 : Further PEP 667 docs updates

654abde

* Clarify impact on default behaviour of exec, eval, etc * Update documentation for changes to PyEval_GetLocals (pythongh-74929) Closes pythongh-11888

ncoghlan mentioned this pull request Jun 1, 2024

gh-118888: Further PEP 667 docs updates #119893

Merged

ncoghlan added a commit that referenced this pull request Jun 1, 2024

gh-118888 : Further PEP 667 docs updates (gh-119893 )

2180991

* Clarify impact on default behaviour of exec, eval, etc * Update documentation for changes to PyEval_GetLocals (gh-74929) Closes gh-11888

miss-islington mentioned this pull request Jun 1, 2024

[3.13] gh-118888: Further PEP 667 docs updates (gh-119893) #119894

Merged

bpo-36004: Add date.fromisocalendar by pganssle · Pull Request #11888 · python/cpython · GitHub

Uh oh!

Uh oh!

bpo-36004: Add date.fromisocalendar - #11888

bpo-36004: Add date.fromisocalendar#11888vstinner merged 14 commits intopython:masterpython/cpython:masterfrom pganssle:add_fromisoformatpganssle/cpython:add_fromisoformatCopy head branch name to clipboard

Conversation

pganssle commented Feb 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented Feb 17, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pganssle Feb 17, 2019

Choose a reason for hiding this comment

Uh oh!

pganssle commented Feb 18, 2019

Uh oh!

bedevere-bot commented Feb 18, 2019

Uh oh!

pablogsal left a comment

Choose a reason for hiding this comment

Uh oh!

pganssle commented Mar 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

vstinner Apr 28, 2019

Choose a reason for hiding this comment

Uh oh!

pganssle Apr 29, 2019

Choose a reason for hiding this comment

Uh oh!

vstinner Apr 28, 2019

Choose a reason for hiding this comment

Uh oh!

pganssle Apr 29, 2019

Choose a reason for hiding this comment

Uh oh!

vstinner commented Apr 29, 2019

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner commented Apr 29, 2019

Uh oh!

pganssle commented Apr 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bpo-36004: Add date.fromisocalendar#11888
vstinner merged 14 commits into
python:masterfrom
pganssle:add_fromisoformat

pganssle commented Feb 16, 2019 •
edited
Loading

pganssle commented Mar 17, 2019 •
edited
Loading