gh-121237: Add %:z directive to datetime.strptime
#136961
Merged
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
donBarbos
changed the title
%:z directive to strptime%:z directive to datetime.strptime
pganssle
reviewed
Sep 15, 2025
Misc/NEWS.d/next/Library/2025-07-21-20-00-42.gh-issue-121237.DyxNqo.rst
Outdated
Show resolved
Hide resolved
Misc/NEWS.d/next/Library/2025-07-21-20-00-42.gh-issue-121237.DyxNqo.rst
Outdated
Show resolved
Hide resolved
|
I think the changes for |
I don't understand, this is a feature now and cannot be backported? |
Sorry, but it seemed to me that the features are not being backported. |
I think @donBarbos is suggesting that we update the docs for |
Yes, that’s exactly what I meant. Sorry, my English and wording are not always clear. |
Comment on lines
445
to
447
| with self.assertRaises(ValueError) as err: | ||
| _strptime._strptime("-01:3030", "%z") | ||
| self.assertEqual("Inconsistent use of : in -01:3030", str(err.exception)) |
There was a problem hiding this comment.
The error message should be improved for this case, i.e. lack of colons, (see my previous review for the current state) and tested in this pr IMO.
There was a problem hiding this comment.
After thinking about it, I realized that the “unconverted data remains” error does not happen with %:z because, in your example, the string doesn’t fully match the expected format. With %z, however, the same string is considered valid. The difference comes from how the use of colons is handled, this consistency check is specific to %z, since %:z always requires colons by definition (it's by regex rule).
I can probably add a hack to check for this, but it seems like it's just not the right message since it's an additional check to match and it would be weird to use an additional rule for %z to %:z.
There was a problem hiding this comment.
I understand why you suggested that (because it seems to indicate a lack of colons), but it's actually a %z-specific check that checks for consistency.
There was a problem hiding this comment.
Oof. This does seem symmetrical with the %z case, but it does seem annoying to fix in the current regex-based system. I think special-casing it is fine as long as it's not too messy, but if there's no clean way to do it we can maybe just accept this and see if anyone complains.
There was a problem hiding this comment.
For reference, it currently raises:
>>> datetime.strptime("-01:3030", "%:z")
ValueError: unconverted data remains: 30
I have a good feeling people will complain, as the message isn't helpful and also makes it seem like %:z doesn't support seconds.
There was a problem hiding this comment.
I accepted all suggestions, including trying to carefully write a hack for this case, and taking advantage of the fact that the error was actually different and in other place, I clarified what the error was.
But I also want to note that if the : between HH and MM is missing (for example -0130:30), the error will be uncovered data ..., but for %z there is also a different error (ValueError: invalid literal for int() with base 10: ':3').
There was a problem hiding this comment.
I have a good feeling people will complain, as the message isn't helpful and also makes it seem like
%:zdoesn't support seconds.
I suspect you are right, but also I am kind of hoping that at some point in the future we will do a teardown rewrite of strftime and strptime that uses a state machine or something else other than a bunch of regexes, at which point it would be much easier to make these errors nice (and probably make strptime/strftime dramatically faster, and I think that this PR is an improvement over the status quo even with this particular corner case having a funky error message.
pganssle
requested changes
Sep 17, 2025
There was a problem hiding this comment.
Ok I think this is most of the way ready to go. One thing I think I see missing is that the main difference between %:z and %z is that %:z is more strict about colons, but there's no test of that strictness — all the tests that apply to %:z also apply to %z, no?
Maybe add some cases where %:z fails on things like -1000 or whatever.
Comment on lines
445
to
447
| with self.assertRaises(ValueError) as err: | ||
| _strptime._strptime("-01:3030", "%z") | ||
| self.assertEqual("Inconsistent use of : in -01:3030", str(err.exception)) |
There was a problem hiding this comment.
Oof. This does seem symmetrical with the %z case, but it does seem annoying to fix in the current regex-based system. I think special-casing it is fine as long as it's not too messy, but if there's no clean way to do it we can maybe just accept this and see if anyone complains.
|
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 |
* Update tests * Add test cases for missing colons * Check missing colons
|
I have made the requested changes; please review again |
|
Thanks for making the requested changes! @pganssle: please review the changes made to this pull request. |
pganssle
approved these changes
Sep 18, 2025
There was a problem hiding this comment.
Great job as usual @donBarbos and thanks @StanFromIreland for helping with the review. Thanks also @LucasEsposito for kicking this off.
|
It seems there is no need for a separate PR for time docs, I was unable to use the
>>> time.strftime("%H:%M:%S %:z")
'22:47:22 %:z'
>>> time.strftime("%H:%M:%S %z")
'22:47:25 +0400'
>>> time.strptime("11:50:49 +02:30", "%H:%M:%S %:z")
time.struct_time(tm_year=1900, tm_mon=1, tm_mday=1, tm_hour=11, tm_min=50, tm_sec=49, tm_wday=0, tm_yday=1, tm_isdst=-1)
>>> time.strptime("11:50:49 +02:30", "%H:%M:%S %z")
time.struct_time(tm_year=1900, tm_mon=1, tm_mday=1, tm_hour=11, tm_min=50, tm_sec=49, tm_wday=0, tm_yday=1, tm_isdst=-1)
>>> time.strptime("11:50:49", "%H:%M:%S")
time.struct_time(tm_year=1900, tm_mon=1, tm_mday=1, tm_hour=11, tm_min=50, tm_sec=49, tm_wday=0, tm_yday=1, tm_isdst=-1) |
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Thanks to @LucasEsposito for the initial PR
cc @zuo @StanFromIreland as previous reviewers
%:zindatetime.datetime.strptime#121237📚 Documentation preview 📚: https://cpython-previews--136961.org.readthedocs.build/