gh-64376: Use Argument Clinic for datetime.date classmethods by hauntsaninja · Pull Request #20208 · python/cpython

hauntsaninja · 2020-05-19T06:28:08Z

https://bugs.python.org/issue20177

Issue: Derby #8: Convert 28 sites to Argument Clinic across 2 files #64376

Note that this will now throw OverflowError instead of ValueError when using the C version of datetime. There is precedent for this, e.g., datetime.date.fromordinal will throw an OverflowError with the C module but ValueError with the Python.

hauntsaninja · 2020-05-19T06:29:03Z

Re: datetime.date.fromisocalendar
Note that this now can throw OverflowError instead of ValueError when
using the C version of datetime. There is precedent for this, e.g.,
datetime.date.fromordinal will throw an OverflowError with the C module
but ValueError with the Python.
Also, prior to this PR, the C version fromisocalendar would throw a ValueError while the Python version throws TypeError for issues with e.g, missing arguments.

hauntsaninja · 2020-05-19T06:30:50Z

Two small notes:

Let me know if this diff size is too small and piece-meal-y!
I like the docstrings in Lib/datetime.py better, but unless otherwise told, I'll save that for another issue.

taleinat · 2020-10-03T09:48:47Z

I like the docstrings in Lib/datetime.py better, but unless otherwise told, I'll save that for another issue.

Indeed, it's good to keep these Argument Clinic conversions as straightforward as possible.

taleinat · 2020-10-03T09:56:56Z

Closing and re-opening to restart the hanged Travis-CI check.

hauntsaninja · 2020-10-05T01:00:45Z

Thanks, looks like everything's green :-)

taleinat

Thanks for this!

Needs a few tweaks but overall looks good.

taleinat · 2022-01-20T22:41:57Z

Lib/test/datetimetester.py

-            (2<<32, 1, 1),
-            (2019, 2<<32, 1),
-            (2019, 1, 2<<32),


Why are these removed?

I mentioned this in a PR comment:

Re: datetime.date.fromisocalendar
Note that this now can throw OverflowError instead of ValueError when
using the C version of datetime. There is precedent for this, e.g.,
datetime.date.fromordinal will throw an OverflowError with the C module
but ValueError with the Python.
Also, prior to this PR, the C version fromisocalendar would throw a ValueError while the Python version throws TypeError for issues with e.g, missing arguments.

Ah, thanks, I didn't make the connection! I'll take another look at that.

Misc/NEWS.d/next/Library/2020-05-19-06-32-11.bpo-20177.BwGrIX.rst

Modules/_datetimemodule.c

bedevere-bot · 2022-01-21T09:00:15Z

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.

Co-authored-by: Tal Einat <[email protected]>

Modules/_datetimemodule.c

hauntsaninja

Thanks for the review! :-)

taleinat · 2022-01-21T12:21:36Z

Let me know if this diff size is too small and piece-meal-y!

In my opinion this is fine, huge PRs can be hard to work with.

taleinat · 2022-01-21T12:32:24Z

Modules/_datetimemodule.c

-        if (PyErr_ExceptionMatches(PyExc_OverflowError)) {
-            PyErr_Format(PyExc_ValueError,
-                    "ISO calendar component out of range");
+    year: int
+    week: int
+    day: int

-        }


Now I see that before this PR we have code in fromisocalendar() to raise ValueError rather than OverflowError. Raising a ValueError seems like better behavior to me. Also, changing this would be considered backwards-incompatible, would require NEWS and What's New entries, and couldn't be backported.

I think we should find a way to retain this behavior, even if that means not using Argument Clinic for this method right now.

taleinat · 2022-01-21T12:37:30Z

To respond to all points re. types of exceptions raised:

Note that this now can throw OverflowError instead of ValueError when using the C version of datetime.

As I stated above, I suggest keeping the behavior of raising ValueError rather than OverflowError.

There is precedent for this, e.g., datetime.date.fromordinal will throw an OverflowError with the C module but ValueError with the Python.

Changing this to raise ValueError instead of OverflowError would be an improvement IMO.

Also, prior to this PR, the C version fromisocalendar would throw a ValueError while the Python version throws TypeError for issues with e.g, missing arguments.

These inconsistencies are unfortunate, but relatively minor. If this PR fixes that, adding a test for it would be great!

arhadthedev · 2023-04-19T12:19:16Z

@hauntsaninja could you fix merge conflicts, please?

hauntsaninja added 4 commits May 18, 2020 23:02

datetime.date.fromordinal

ee232ca

datetime.date.fromisoformat

78e71d0

datetime.date.fromisocalendar

d0eac8d

Note that this will now throw OverflowError instead of ValueError when using the C version of datetime. There is precedent for this, e.g., datetime.date.fromordinal will throw an OverflowError with the C module but ValueError with the Python.

datetime.today

5d9b7de

hauntsaninja requested review from abalkin and pganssle as code owners May 19, 2020 06:28

the-knights-who-say-ni added the CLA signed label May 19, 2020

bedevere-bot added the awaiting review label May 19, 2020

blurb-it bot and others added 3 commits May 19, 2020 06:32

📜🤖 Added by blurb_it.

0497383

Merge remote-tracking branch 'upstream/master' into datetime

13ad7c7

re-run argument clinic

c1a2b1e

taleinat closed this Oct 3, 2020

taleinat reopened this Oct 3, 2020

taleinat added the skip news label Jan 20, 2022

taleinat requested changes Jan 21, 2022

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Jan 21, 2022

Update Modules/_datetimemodule.c

e6f7572

Co-authored-by: Tal Einat <[email protected]>

taleinat reviewed Jan 21, 2022

View reviewed changes

Modules/_datetimemodule.c Outdated Show resolved Hide resolved

hauntsaninja added 3 commits January 21, 2022 01:13

Remove NEWS entry

11f83fd

Fix datetime.date.today docstring

188130b

Re-run clinic

0528066

hauntsaninja commented Jan 21, 2022

View reviewed changes

Use dtstr instead of date_string

d6c8759

taleinat reviewed Jan 21, 2022

View reviewed changes

serhiy-storchaka added the topic-argument-clinic label Apr 30, 2022

ezio-melotti removed the CLA signed label Jul 13, 2022

larryhastings mentioned this pull request Apr 10, 2022

Derby #8: Convert 28 sites to Argument Clinic across 2 files #64376

Closed

arhadthedev changed the title ~~bpo-20177: Use Argument Clinic for datetime.date classmethods~~ gh-64376: Use Argument Clinic for datetime.date classmethods Apr 19, 2023

hauntsaninja closed this Mar 6, 2024

hauntsaninja deleted the datetime branch March 6, 2024 22:40

Uh oh!

Conversation

hauntsaninja commented May 19, 2020 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hauntsaninja commented May 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hauntsaninja commented May 19, 2020

Uh oh!

taleinat commented Oct 3, 2020

Uh oh!

taleinat commented Oct 3, 2020

Uh oh!

hauntsaninja commented Oct 5, 2020

Uh oh!

taleinat left a comment

Choose a reason for hiding this comment

Uh oh!

taleinat Jan 20, 2022

Choose a reason for hiding this comment

Uh oh!

hauntsaninja Jan 21, 2022

Choose a reason for hiding this comment

Uh oh!

taleinat Jan 21, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented Jan 21, 2022

Uh oh!

Uh oh!

hauntsaninja left a comment

Choose a reason for hiding this comment

Uh oh!

taleinat commented Jan 21, 2022

Uh oh!

taleinat Jan 21, 2022

Choose a reason for hiding this comment

Uh oh!

taleinat commented Jan 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arhadthedev commented Apr 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

hauntsaninja commented May 19, 2020 •

edited by bedevere-bot

Loading

hauntsaninja commented May 19, 2020 •

edited

Loading

taleinat commented Jan 21, 2022 •

edited

Loading