GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-36004: Add date.fromisocalendar by pganssle · Pull Request #11888 · python/cpython

Conversation

@pganssle

@pganssle pganssle commented Feb 16, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Feb 16, 2019
@bedevere-bot bedevere-bot added the awaiting review label Feb 16, 2019
pablogsal
pablogsal previously requested changes Feb 17, 2019
View reviewed changes
Comment thread Modules/_datetimemodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting changes and removed awaiting review labels Feb 17, 2019
@bedevere-bot

bedevere-bot commented Feb 17, 2019

Copy link
Copy Markdown

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
pganssle
pganssle commented Feb 17, 2019
View reviewed changes
Comment thread Lib/datetime.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
ZackerySpytz
ZackerySpytz reviewed Feb 17, 2019
View reviewed changes
Comment thread Modules/_datetimemodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_datetimemodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@pganssle
pganssle force-pushed the add_fromisoformat branch 3 times, most recently from 221f2a7 to 3d3e6e7 Compare February 17, 2019 17:38
@pganssle pganssle changed the title bpo-36004: Add date.fromisocalendar and tests bpo-36004: Add date.fromisocalendar Feb 17, 2019
pganssle
pganssle commented Feb 17, 2019
View reviewed changes
Comment thread Lib/test/datetimetester.py Outdated

@pganssle pganssle Feb 17, 2019

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@pganssle

pganssle commented Feb 18, 2019

Copy link
Copy Markdown
Member Author

I have made the requested changes; please review again.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting change review and removed awaiting changes labels Feb 18, 2019
@bedevere-bot

bedevere-bot commented Feb 18, 2019

Copy link
Copy Markdown

Thanks for making the requested changes!

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
pablogsal
pablogsal reviewed Mar 16, 2019
View reviewed changes

@pablogsal pablogsal left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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 in Doc/whatsnew/3.8.rst

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@pganssle
pganssle force-pushed the add_fromisoformat branch from ba63b21 to 44f9adf Compare March 17, 2019 19:18
@pganssle

pganssle commented Mar 17, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@pganssle
pganssle force-pushed the add_fromisoformat branch from 44f9adf to 889f58c Compare March 17, 2019 19:44
@pganssle
pganssle force-pushed the add_fromisoformat branch from 889f58c to a9ba64d Compare April 3, 2019 19:05
@pganssle
pganssle force-pushed the add_fromisoformat branch from a9ba64d to c514f03 Compare April 13, 2019 00:14
@pganssle
pganssle force-pushed the add_fromisoformat branch from c514f03 to 5f40dd5 Compare April 20, 2019 18:31
@pablogsal
pablogsal dismissed their stale review April 27, 2019 16:32

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

@pganssle
pganssle force-pushed the add_fromisoformat branch from 5f40dd5 to 535a713 Compare April 27, 2019 16:54
vstinner
vstinner reviewed Apr 28, 2019
View reviewed changes
Comment thread Lib/datetime.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/datetimetester.py Outdated

@vstinner vstinner Apr 28, 2019

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@pganssle pganssle Apr 29, 2019

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/datetimetester.py Outdated

@vstinner vstinner Apr 28, 2019

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@pganssle pganssle Apr 29, 2019

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@vstinner

vstinner commented Apr 29, 2019

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
pganssle added 2 commits April 29, 2019 07:43
@pganssle
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
@pganssle
Add early year check to fromisocalendar
fbab728 
This avoids an overflow error in ordinal calculations in the C
implementation.
pganssle added 12 commits April 29, 2019 07:43
@pganssle
Covert OverflowError into ValueError
925de2f 
All overflow errors in this context are also value errors, so it is
preferable to be uniform in the type of error raised.
@pganssle
Add bounds checking for ISO long vs. short weeks
3ecf02e
@pganssle
Add more comments with test cases
e645cd0
@pganssle
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.
@pganssle
Fix missing argument in PyErr_Format
74fb523
@pganssle
Add test for TypeError handling
46c6458
@pganssle
Add documentation for date(time).fromisocalendar
28c7fb2
@pganssle
Add NEWS entry
99e75d7
@pganssle
Add What's New entry for fromisocalendar
c269a5c
@pganssle
Use MINYEAR and MAXYEAR in bounds checks
c8444b7
@pganssle
Add tests for MINYEAR and MAXYEAR
958ffff
@pganssle
Use generated test cases instead of manual list
3307865
@pganssle
pganssle force-pushed the add_fromisoformat branch from 535a713 to 3307865 Compare April 29, 2019 12:00
@pganssle pganssle closed this Apr 29, 2019
@pganssle pganssle reopened this Apr 29, 2019
vstinner
vstinner approved these changes Apr 29, 2019
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting change review labels Apr 29, 2019
@vstinner

vstinner commented Apr 29, 2019

Copy link
Copy Markdown
Member

Azure Pipelines PR — #20190429.29 failed

Random network issue, unrelated to this change.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner
vstinner merged commit 88c0937 into python:master Apr 29, 2019
@bedevere-bot bedevere-bot removed the awaiting merge label Apr 29, 2019
@pganssle

pganssle commented Apr 29, 2019

Copy link
Copy Markdown
Member Author

@vstinner Thanks for the review and merge, Victor!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ncoghlan added a commit to ncoghlan/cpython that referenced this pull request Jun 1, 2024
@ncoghlan
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 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
@ncoghlan
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 pushed a commit to miss-islington/cpython that referenced this pull request Jun 1, 2024
@ncoghlan @miss-islington
pythongh-118888: Further PEP 667 docs updates (pythongh-119893)
4760402 
* 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 <[email protected]>
@miss-islington miss-islington mentioned this pull request Jun 1, 2024
[3.13] gh-118888: Further PEP 667 docs updates (gh-119893) #119894
Merged
barneygale pushed a commit to barneygale/cpython that referenced this pull request Jun 5, 2024
@ncoghlan @barneygale
pythongh-118888: Further PEP 667 docs updates (pythongh-119893)
1e0b92e 
* Clarify impact on default behaviour of exec, eval, etc
* Update documentation for changes to PyEval_GetLocals (pythongh-74929)

Closes pythongh-11888
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
@ncoghlan @estyxx
pythongh-118888: Further PEP 667 docs updates (pythongh-119893)
ed4f826 
* Clarify impact on default behaviour of exec, eval, etc
* Update documentation for changes to PyEval_GetLocals (pythongh-74929)

Closes pythongh-11888
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pablogsal pablogsal pablogsal left review comments
@vstinner vstinner vstinner approved these changes
+1 more reviewer
@ZackerySpytz ZackerySpytz ZackerySpytz left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@pganssle @bedevere-bot @vstinner @pablogsal @ZackerySpytz @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.