Skip to content
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

bpo-36004: Add date.fromisocalendar #11888

Merged
merged 14 commits into from Apr 29, 2019

Conversation

@pganssle
Copy link
Member

commented Feb 16, 2019

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

Modules/_datetimemodule.c Outdated Show resolved Hide resolved
@bedevere-bot

This comment has been minimized.

Copy link

commented Feb 17, 2019

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.

Lib/datetime.py Outdated Show resolved Hide resolved
Modules/_datetimemodule.c Outdated Show resolved Hide resolved
Modules/_datetimemodule.c Outdated Show resolved Hide resolved

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

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

(None, 1, 1),
(2019, None, 1),
(2019, 1, None),
(2019.0, 1, 1),

This comment has been minimized.

Copy link
@pganssle

pganssle Feb 17, 2019

Author Member

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.

@pganssle

This comment has been minimized.

Copy link
Member Author

commented Feb 18, 2019

I have made the requested changes; please review again.

@bedevere-bot

This comment has been minimized.

Copy link

commented Feb 18, 2019

Thanks for making the requested changes!

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

@pablogsal
Copy link
Member

left a comment

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

@pganssle pganssle force-pushed the pganssle:add_fromisoformat branch from ba63b21 to 44f9adf Mar 17, 2019

@pganssle

This comment has been minimized.

Copy link
Member Author

commented Mar 17, 2019

@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.

@pganssle pganssle force-pushed the pganssle:add_fromisoformat branch from 44f9adf to 889f58c Mar 17, 2019

@pganssle pganssle force-pushed the pganssle:add_fromisoformat branch from 889f58c to a9ba64d Apr 3, 2019

@pganssle pganssle force-pushed the pganssle:add_fromisoformat branch from a9ba64d to c514f03 Apr 13, 2019

@pganssle pganssle force-pushed the pganssle:add_fromisoformat branch from c514f03 to 5f40dd5 Apr 20, 2019

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

@pganssle pganssle force-pushed the pganssle:add_fromisoformat branch from 5f40dd5 to 535a713 Apr 27, 2019

Lib/datetime.py Outdated Show resolved Hide resolved
(2004, 1, 1), # Leap year
(2004, 12, 31),
(1, 1, 1),
(9999, 12, 31),

This comment has been minimized.

Copy link
@vstinner

vstinner Apr 28, 2019

Member

Maybe use MINYEAR/MAXYEAR here.

This comment has been minimized.

Copy link
@pganssle

pganssle Apr 29, 2019

Author Member

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.

(2019.0, 1, 1),
(2019, 1.0, 1),
(2019, 1, 1.0),
]

This comment has been minimized.

Copy link
@vstinner

vstinner Apr 28, 2019

Member

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

This comment has been minimized.

Copy link
@pganssle

pganssle Apr 29, 2019

Author Member

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.

@vstinner

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

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.

pganssle added 2 commits Feb 15, 2019
Add date.fromisocalendar and tests
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
This avoids an overflow error in ordinal calculations in the C
implementation.
pganssle added 12 commits Feb 17, 2019
Covert OverflowError into ValueError
All overflow errors in this context are also value errors, so it is
preferable to be uniform in the type of error raised.
Switch to new method of detecting ISO long years
This is equivalent but uses only existing helper functions and in many
cases will be slightly more efficient.

@pganssle pganssle force-pushed the pganssle:add_fromisoformat branch from 535a713 to 3307865 Apr 29, 2019

@pganssle pganssle closed this Apr 29, 2019

@pganssle pganssle reopened this Apr 29, 2019

@vstinner
Copy link
Member

left a comment

LGTM.

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

@vstinner

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

Azure Pipelines PR — #20190429.29 failed

Random network issue, unrelated to this change.

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

4 of 5 checks passed

Azure Pipelines PR #20190429.29 failed
Details
bedevere/issue-number Issue number 36004 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pganssle

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2019

@vstinner Thanks for the review and merge, Victor!

arnolddumas added a commit to arnolddumas/cpython that referenced this pull request May 3, 2019
bpo-36004: Add date.fromisocalendar (pythonGH-11888)
This commit implements the first version of date.fromisocalendar, the
inverse function for date.isocalendar.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.