Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

bpo-36004: Add date.fromisocalendar#11888

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
vstinner merged 14 commits intopython:masterfrompganssle:add_fromisoformat
Apr 29, 2019

Conversation

@pganssle
Copy link
Member

@pgansslepganssle commentedFeb 16, 2019•
edited
Loading

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

To Do:

  • Validate ISO datetimes
  • Test forTypeError
  • 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

pablogsal
pablogsal previously requested changesFeb 17, 2019
@bedevere-bot
Copy link

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

@pgansslepganssleforce-pushed theadd_fromisoformat branch 3 times, most recently from221f2a7 to3d3e6e7CompareFebruary 17, 2019 17:38
@pgansslepganssle changed the titlebpo-36004: Add date.fromisocalendar and testsbpo-36004: Add date.fromisocalendarFeb 17, 2019
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

One thing I'll note is that this is consistent withdatetime.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
Copy link
MemberAuthor

I have made the requested changes; please review again.

@bedevere-bot
Copy link

Thanks for making the requested changes!

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

Copy link
Member

@pablogsalpablogsal left a comment

Choose a reason for hiding this comment

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

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

@pganssle
Copy link
MemberAuthor

pganssle commentedMar 17, 2019•
edited
Loading

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

@pablogsalpablogsal dismissed theirstale reviewApril 27, 2019 16:32

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

Copy link
Member

Choose a reason for hiding this comment

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

Maybe use MINYEAR/MAXYEAR here.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

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
Copy link
Member

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, theinverse function for date.isocalendar. It is currently missing errorchecking 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 Cimplementation.
Copy link
Member

@vstinnervstinner left a comment

Choose a reason for hiding this comment

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

LGTM.

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

@vstinner
Copy link
Member

Azure Pipelines PR — #20190429.29 failed

Random network issue, unrelated to this change.

@vstinnervstinner merged commit88c0937 intopython:masterApr 29, 2019
@pganssle
Copy link
MemberAuthor

@vstinner Thanks for the review and merge, Victor!

ncoghlan added a commit to ncoghlan/cpython that referenced this pull requestJun 1, 2024
* Clarify impact on default behaviour of exec, eval, etc* Update documentation for changes to PyEval_GetLocals (pythongh-74929)Closespythongh-11888
ncoghlan added a commit that referenced this pull requestJun 1, 2024
* Clarify impact on default behaviour of exec, eval, etc* Update documentation for changes to PyEval_GetLocals (gh-74929)Closesgh-11888
miss-islington pushed a commit to miss-islington/cpython that referenced this pull requestJun 1, 2024
* Clarify impact on default behaviour of exec, eval, etc* Update documentation for changes to PyEval_GetLocals (pythongh-74929)Closespythongh-11888(cherry picked from commit2180991)Co-authored-by: Alyssa Coghlan <ncoghlan@gmail.com>
barneygale pushed a commit to barneygale/cpython that referenced this pull requestJun 5, 2024
* Clarify impact on default behaviour of exec, eval, etc* Update documentation for changes to PyEval_GetLocals (pythongh-74929)Closespythongh-11888
noahbkim pushed a commit to hudson-trading/cpython that referenced this pull requestJul 11, 2024
* Clarify impact on default behaviour of exec, eval, etc* Update documentation for changes to PyEval_GetLocals (pythongh-74929)Closespythongh-11888
estyxx pushed a commit to estyxx/cpython that referenced this pull requestJul 17, 2024
* Clarify impact on default behaviour of exec, eval, etc* Update documentation for changes to PyEval_GetLocals (pythongh-74929)Closespythongh-11888
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@pablogsalpablogsalpablogsal left review comments

@vstinnervstinnervstinner approved these changes

+1 more reviewer

@ZackerySpytzZackerySpytzZackerySpytz left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@pganssle@bedevere-bot@vstinner@pablogsal@ZackerySpytz@the-knights-who-say-ni
[8]ページ先頭
©2009-2025 Movatter.jp