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-70647: Deprecate strptime day of month parsing without a year present to avoid leap-year bugs #117107

Merged
merged 9 commits into from
Apr 3, 2024

Conversation

gpshead
Copy link
Member

@gpshead gpshead commented Mar 20, 2024

If we don't do this now, people will still be using Python releases that don't guide code away from the bug by the time the next leap year rolls around. :)

Decision on what our actual breaking change will be has been left for later, this just starts the process to allow us to do it with a suggestion of how to always avoid such problems in code.

TODO list:

year_in_format = True
case 'd':
day_of_month_in_format = True
if day_of_month_in_format and not year_in_format:
Copy link
Member Author

@gpshead gpshead Mar 20, 2024

Choose a reason for hiding this comment

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

Strictly speaking this warns when a %d day-of-month is being parsed when there is no indicator of what the Month is (in which case the default of January presumably allows the day to go up to 31) but i presume such formats are unusual and thus rare enough to elide extra logic for and warn on those anyways. Being more pedantic would require one more bool in the conditional and the number of strptime format characters representing some form of month is large/ugly.

Copy link
Member

Choose a reason for hiding this comment

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

I agree.

Copy link
Member

Choose a reason for hiding this comment

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

I would be mildly in favor of deprecating all these ambiguous kinds of formats anyway, though I do think "add a fixed year to the string to be parsed" is an uglier hack than adding some mechanism for explicitly specifying defaults (e.g. accepting a default_datetime: datetime | None parameter, or default_kwargs={...}).

Which I suppose is my way of saying that I generally agree with this — I suspect that any format that has a year and a day-of-month without an actual month is way more likely to be due to an error than not. Preferably we'd give people a more accurate warning in that case, but for now any kind of warning to call attention to the situation should help.

@gpshead gpshead requested a review from Yhg1s March 28, 2024 22:03
@gpshead
Copy link
Member Author

gpshead commented Mar 29, 2024

Documentation added, but I need to reconcile this with my proposed non deprecation related wording drafted in #116179.

To passively encourage people to include such comments in their own code
when adopting this mitigation...
@gpshead gpshead requested a review from encukou April 1, 2024 23:38
Doc/library/datetime.rst Outdated Show resolved Hide resolved
Lib/_strptime.py Outdated
To avoid trouble, add a specific year to the input & format.
See https://github.com/python/cpython/issues/70647.""",
DeprecationWarning,
stacklevel=5)
Copy link
Member

Choose a reason for hiding this comment

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

The 5 looks fragile, but I don't see a good way around it, short of teaching warnings new tricks.
Hopefully this code won't get refactored in the next few years.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, wow, nice job with the time machine :)

Suggested change
stacklevel=5)
skip_file_prefixes=(os.path.dirname(__file__),))

Looks like this would be the first use of skip_file_prefixes in the stdlib.

@encukou encukou merged commit 33ee5cb into python:main Apr 3, 2024
33 checks passed
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants