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

gh-64376: Use Argument Clinic for datetime.date classmethods #20208

Closed
wants to merge 12 commits into from

Conversation

hauntsaninja
Copy link
Contributor

@hauntsaninja hauntsaninja commented May 19, 2020

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.
@hauntsaninja
Copy link
Contributor Author

hauntsaninja commented May 19, 2020

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
Copy link
Contributor Author

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.

@taleinat
Copy link
Contributor

taleinat commented Oct 3, 2020

  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.

@taleinat
Copy link
Contributor

taleinat commented Oct 3, 2020

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

@taleinat taleinat closed this Oct 3, 2020
@taleinat taleinat reopened this Oct 3, 2020
@hauntsaninja
Copy link
Contributor Author

Thanks, looks like everything's green :-)

Copy link
Contributor

@taleinat taleinat left a comment

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
(2<<32, 1, 1),
(2019, 2<<32, 1),
(2019, 1, 2<<32),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these removed?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Modules/_datetimemodule.c Outdated Show resolved Hide resolved
Modules/_datetimemodule.c Outdated Show resolved Hide resolved
@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 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.

Modules/_datetimemodule.c Outdated Show resolved Hide resolved
Copy link
Contributor Author

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Thanks for the review! :-)

@taleinat
Copy link
Contributor

  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
if (PyErr_ExceptionMatches(PyExc_OverflowError)) {
PyErr_Format(PyExc_ValueError,
"ISO calendar component out of range");
year: int
week: int
day: int

}
Copy link
Contributor

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.

@taleinat
Copy link
Contributor

taleinat commented Jan 21, 2022

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

@hauntsaninja could you fix merge conflicts, please?

@hauntsaninja hauntsaninja deleted the datetime branch March 6, 2024 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants