8000 GH-90750: Use datetime.fromisocalendar in _strptime by pganssle · Pull Request #103802 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

GH-90750: Use datetime.fromisocalendar in _strptime #103802

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 27, 2023
Merged

GH-90750: Use datetime.fromisocalendar in _strptime #103802

merged 1 commit into from
Apr 27, 2023

Conversation

pganssle
Copy link
Member
@pganssle pganssle commented Apr 24, 2023 •
edited by bedevere-bot
Loading

This unifies the ISO → Gregorian conversion logic and improves handling of invalid ISO weeks.

@pganssle pganssle requested a review from abalkin as a code owner April 24, 2023 22:02
@bedevere-bot bedevere-bot mentioned this pull request Apr 24, 2023
Closed
@encukou
Copy link
Member
encukou commented Apr 25, 2023

It looks like this breaks both backwards compatibility and the documented behaviour of %W and %V (“All days in a new year preceding the first Sunday are considered to be in week 0.”)

@pganssle
Copy link
Member Author
pganssle commented Apr 25, 2023 •
edited
Loading

It looks like this breaks both backwards compatibility

Maybe for some interpretations of backwards compatibility, but this doesn't break any existing tests and it seems like a bug that strptime accepts some (but not all) invalid ISO weeks. I'm willing to hear counter-arguments, but it seems to me that if we consider any observable change to the behavior of a system to be backwards incompatible, we'd never be able to fix any bugs at all.

and the documented behaviour of %W and %V (“All days in a new year preceding the first Sunday are considered to be in week 0.”)

That is the documented behavior of %W and %U, not of %V. %V is documented as:

ISO 8601 week as a decimal number with Monday as the first day of the week. Week 01 is the week containing Jan 4.

This should be consistent with fromisocalendar, since fromisocalendar has the same definition.

@encukou
Copy link
Member
encukou commented Apr 25, 2023

if we consider any observable change to the behavior of a system to be backwards incompatible, we'd never be able to fix any bugs at all.

For fixing bugs that need a backwards compatibility break, we have the deprecation process.

Here, given that week 0 is a documented extension to the ISO week numbers, rejecting 2021-53 doesn't seem very clear-cut to me. I don't think many users actually read the standard when they use strptime.

That is the documented behavior of %W and %U, not of %V

Yes, sorry.

@pganssle
Copy link
Member Author
pganssle commented Apr 25, 2023 •
edited
Loading

I'm sorry, can you clarify what you think this breaks? This PR does not affect the handling of week 0. This is the current behavior:

>>> datetime.strptime("2019-01-1", "%G-%V-%u")
datetime.datetime(2018, 12, 31, 0, 0)

>>> datetime.strptime("2019-00-1", "%G-%V-%u")
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
...
ValueError: time data '2019-00-1' does not match format '%G-%V-%u'

>>> datetime.strptime("2019-54-1", "%G-%V-%u")
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
ValueError: time data '2019-54-1' does not match format '%G-%V-%u'

What has changed is that %G-%V-%u no longer accepts 2021-53-1 because week 53 is not a valid ISO week. It's like if %Y-%m-%d were accepting 2023-02-29 and parsing it as 2023-03-01 with no documentation of this behavior (or in fact documentation that the string is expected to be a valid date) and no tests for it.

I don't plan to backport this to 3.11, but I think that this can very reasonably be considered a bug fix. I think you may be thinking of %U and %W as having some bearing on ISO weeks, but that is a totally different system of counting weeks, and this PR doesn't affect them.

@pganssle
Use datetime.fromisocalendar in _strptime
8aee83c 
This unifies the ISO → Gregorian conversion logic and improves handling
of invalid ISO weeks.
@pganssle pganssle force-pushed the replace_iso_calculation branch from cd62fb1 to 8aee83c Compare April 26, 2023 21:38
@FFY00
Copy link
Member
FFY00 commented Apr 26, 2023

+1 for me on the fix itself. I think it can be considered a bugfix, but I also think it has a fair chance of breaking user code. Unless it's a lot of trouble, I'd probably keep the old logic with a DeprecationWarning on the cases that are now invalid, assuming this doesn't change the interpretation of any other values, per Paul's last comment.

encukou
encukou approved these changes Apr 27, 2023
View reviewed changes
Copy link
Member
@encukou encukou left a comment

Choose a reason for hiding this comment

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

Ah, I see now!
Sorry for the noise, and thanks for your patience explaining it.

@pganssle
Copy link
Member Author

No worries! I think I am going to skip the DeprecationWarning thing, since that seems like it would be... complicated, and I think this will only affect a narrow range of edge cases anyway (not even sure what would generate dates like this).

@pganssle pganssle merged commit a5308e1 into python:main Apr 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@encukou encukou encukou approved these changes

@abalkin abalkin Awaiting requested review from abalkin abalkin is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pganssle @encukou @FFY00 @bedevere-bot
0