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

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

hauntsaninja added 4 commits

May 18, 2020 23:02

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.

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.

Two small notes:

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

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

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

Thanks, looks like everything's green :-)

Choose a reason for hiding this comment

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

Thanks for this!

Needs a few tweaks but overall looks good.

Comment on lines -1912 to -1914

Choose a reason for hiding this comment

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

Why are these removed?

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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

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 532281+taleinat@users.noreply.github.com

hauntsaninja added 3 commits

January 21, 2022 01:13

Choose a reason for hiding this comment

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

Thanks for the review! :-)

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

Comment on lines -3020 to -3024

Choose a reason for hiding this comment

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

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.

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 changed the titlebpo-20177: Use Argument Clinic for datetime.date classmethods gh-64376: Use Argument Clinic for datetime.date classmethods

Apr 19, 2023