-
-
Notifications
You must be signed in to change notification settings - Fork 33.2k
gh-121237: Add %:z directive to strptime #122142
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-121237: Add %:z directive to strptime #122142
Conversation
f6227ad to
43a8c13
Compare
| @@ -0,0 +1 @@ | |||
| Accept "%:z" in strptime | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When releasing, changes will be added to the changelog. You should describe your changes in detail but keep it concise. For example:
Added %:Z used for XXX to strptime.
(I'm sorry, I don't know what %:z does).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I like the code :)
- What I propose mainly is to correct/supplement certain docs/tests-related details.
|
|
||
| .. versionadded:: 3.12 | ||
| ``%:z`` was added. | ||
| ``%:z`` was added for :meth:`~.datetime.strftime` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ``%:z`` was added for :meth:`~.datetime.strftime` | |
| ``%:z`` was added for :meth:`~.datetime.strftime`. |
| ``%:z`` was added for :meth:`~.datetime.strftime` | ||
|
|
||
| .. versionadded:: 3.13 | ||
| ``%:z`` was added for :meth:`~.datetime.strptime` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ``%:z`` was added for :meth:`~.datetime.strptime` | |
| ``%:z`` was added for :meth:`~.datetime.strptime`. |
| .. versionadded:: 3.13 | ||
| ``%:z`` was added for :meth:`~.datetime.strptime` | ||
|
|
||
| .. warning:: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, this entire warning is unnecessary, because:
- There is no need to warn, only to describe certain details (see below). After all, the only inconsistency is that
%zis little bit more liberal when it comes to parsing. That's all. - The statement that the behavior is unintended is inaccurate. The behavior is intended and sometimes very useful.
- The recommendation to use
%zdirective only to parse strings formatted according to%zdirective, while using%:zdirective for strings formatted according to%:zis unnecessary (IMHO). If somebody needs to accept only:-based notation, they will parse their input with%:z. If somebody needs to be more liberal in respect to the input, they will parse their input with%z. Both approaches are perfectly OK, so there is no need to discourage people from anything here.
On the other hand, it is worth to describe certain details – but, IMHO, a better place to do so is the note (6) in the Technical Detail subsection (below). Namely, I propose to:
-
(1) Replace the following fragment of the
%zdescription:For example, ``'+01:00:00'`` will be parsed as an offset of one hour. In addition, providing ``'Z'`` is identical to ``'+00:00'``.
with the following text:
For example, both ``'+010000'`` and ``'+01:00:00'`` will be parsed as an offset of one hour. In addition, providing ``'Z'`` is identical to ``'+00:00'``.
-
(2) Replace the following fragment of the
%:zdescription:Behaves exactly as ``%z``, but has a colon separator added between hours, minutes and seconds.with the following text:
When used with :meth:`~.datetime.strftime`, behaves exactly as ``%z``, except that a colon separator is added between hours, minutes and seconds. When used with :meth:`~.datetime.stpftime`, the UTC offset is *required* to have a colon as a separator between hours, minutes and seconds. For example, ``'+01:00:00'`` (but *not* ``'+010000'``) will be parsed as an offset of one hour. In addition, providing ``'Z'`` is identical to ``'+00:00'``.
-
(3) Do not add this Warning frame at all.
| #XXX: Does 'Y' need to worry about having less or more than | ||
| # 4 digits? | ||
| 'Y': r"(?P<Y>\d\d\d\d)", | ||
| # "z" shouldn't support colons. Both ":?" should be removed. However, for backwards |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| # "z" shouldn't support colons. Both ":?" should be removed. However, for backwards | |
| # "z" might not support colons, if designed from scratch. However, for backwards |
Again: the statement that it shoudn't support colons is too strong IMHO. It is perfectly OK, that it supports them.
| self.helper('j', 7) | ||
|
|
||
| def test_offset(self): | ||
| def test_z_directive_offset(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this rename is necessary, considering the surrounding code's method naming convention.
| from ._functools import compose | ||
| from ._itertools import Counter | ||
|
|
||
| from ._test_params import parameterize, Invoked |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cosmetic detail: one more line should be removed here (to leave vertical space of 2 lines, rather than of 3 lines).
| @@ -0,0 +1 @@ | |||
| Accept "%:z" in strptime | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd propose something along the lines:
| Accept "%:z" in strptime | |
| :meth:`~datetime.datetime.strptime` now supports the ``%:z`` format code. |
| self.assertEqual(offset_fraction, -1) | ||
| (*_, offset), _, offset_fraction = _strptime._strptime("+01:00", "%z") | ||
|
|
||
| @parameterize( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test LGTM. :)
Yet, it would also be nice to extend (appropriately) relevant tests in Lib/test/datetimetester.py (which regard both implementations of the datetime module) – namely:
TestDateTime:test_strptime()
TestTime:test_strptime_tz()test_strptime_errors()
I don't think it's a bug. IMHO it's a legitimate part of the module's interface and behavior (even if not to be so useful after merging this PR). And I agree that this PR brings an improvement. :) |
| ``%:z`` was added. | ||
| ``%:z`` was added for :meth:`~.datetime.strftime` | ||
|
|
||
| .. versionadded:: 3.13 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| .. versionadded:: 3.13 | |
| .. versionadded:: 3.14 |
|
What do all the itertools changes have to do with strptime? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has several conflicts and has multiple completely unrelated changes. If you would like to continue with this pr, please tidy it up.
|
@LucasEsposito Thank you for PR, but could you please reply if you're going to continue working on it? (it's just been exactly a year since your last activity in current thread) |
|
I've been meaning to continue working on this for a while, but it just hasn’t happened. Given that I’m currently short on time and this seems to be blocking others from contributing, I’ll step back and leave the issue open so that anyone interested can pick it up. |
|
Closing this in favor of #136961, which is now merged! Thanks everyone! 🎉 |
%:zindatetime.datetime.strptime#121237The directive
%:zwas introduced in Python 3.12. It works as expected onstrftimebut fails onstrptime, since it was not explicitely implemented there as a new directive.This PR adds consistency across
strftimeandstrptime(by implementing%:zon the later), while keeping backwards compatibility.%zand%:zwork as expected instrftime, so nothing was changed there.%zdoes what it should instrptime(accepting strings formatted according to that directive), while in addition allowing%:z-formatted strings too. The later part of the behavior is a bug but at this point we should keep this behavior for backwards compatibility. I documented the issue in the docs, code and tests.%:zfailed when used instrptime. I implemented%:zdirective to parse%:z-formatted strings and added the appropiate tests and changes in the docs.📚 Documentation preview 📚: https://cpython-previews--122142.org.readthedocs.build/