Skip to content

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

LucasEsposito
Copy link
Contributor

@LucasEsposito LucasEsposito commented Jul 22, 2024

The directive %:z was introduced in Python 3.12. It works as expected on strftime but fails on strptime, since it was not explicitely implemented there as a new directive.

This PR adds consistency across strftime and strptime (by implementing %:z on the later), while keeping backwards compatibility.

  • %z and %:z work as expected in strftime, so nothing was changed there.
  • %z does what it should in strptime (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.
  • %:z failed when used in strptime. I implemented %:z directive to parse %:z-formatted strings and added the appropiate tests and changes in the docs.

📚 Documentation preview 📚: https://cpython-previews--122142.org.readthedocs.build/

@LucasEsposito LucasEsposito force-pushed the gh-121237-iso_8601_tz_directive_parsing branch from f6227ad to 43a8c13 Compare July 23, 2024 00:26
@rhettinger rhettinger removed their request for review July 30, 2024 15:23
@@ -0,0 +1 @@
Accept "%:z" in strptime
Copy link
Contributor

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).

Copy link
Contributor

@zuo zuo left a 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.

@@ -2538,7 +2538,21 @@ differences between platforms in handling of unsupported format specifiers.
``%G``, ``%u`` and ``%V`` were added.

.. versionadded:: 3.12
``%:z`` was added.
``%:z`` was added for :meth:`~.datetime.strftime`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
``%: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`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
``%: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::
Copy link
Contributor

@zuo zuo Oct 4, 2024

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 %z is 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 %z directive only to parse strings formatted according to %z directive, while using %:z directive for strings formatted according to %:z is 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 %z description:

    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 %:z description:

    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.

@@ -202,7 +202,10 @@ def __init__(self, locale_time=None):
#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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# "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.

@@ -354,7 +356,7 @@ def test_julian(self):
# Test julian directives
self.helper('j', 7)

def test_offset(self):
def test_z_directive_offset(self):
Copy link
Contributor

@zuo zuo Oct 4, 2024

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

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

@zuo zuo Oct 4, 2024

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:

Suggested change
Accept "%:z" in strptime
:meth:`~datetime.datetime.strptime` now supports the ``%:z`` format code.

@@ -370,22 +372,39 @@ def test_offset(self):
(*_, offset), _, offset_fraction = _strptime._strptime("-013030.000001", "%z")
self.assertEqual(offset, -(one_hour + half_hour + half_minute))
self.assertEqual(offset_fraction, -1)
(*_, offset), _, offset_fraction = _strptime._strptime("+01:00", "%z")

@parameterize(
Copy link
Contributor

@zuo zuo Oct 4, 2024

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()

@zuo
Copy link
Contributor

zuo commented Oct 4, 2024

  • 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.

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

Choose a reason for hiding this comment

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

Suggested change
.. versionadded:: 3.13
.. versionadded:: 3.14

@StanFromIreland
Copy link
Member

What do all the itertools changes have to do with strptime?

Copy link
Member

@StanFromIreland StanFromIreland left a 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.

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.

4 participants