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.

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

bpo-46614: Allow datetime.isoformat to use "Z" UTC designator #32041

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

bpo-46614: Allow datetime.isoformat to use "Z" UTC designator #32041

wants to merge 1 commit into from

Conversation

godlygeek
Copy link
Contributor

@godlygeek godlygeek commented Mar 22, 2022 •
edited by bedevere-bot

Add an optional parameter to datetime.isoformat() and time.isoformat()
called use_utc_designator. If provided and True and the object is
associated with a tzinfo and its tzname is exactly "UTC", the formatted
UTC offset will be "Z" rather than an offset from UTC.

https://bugs.python.org/issue46614

@godlygeek
Allow datetime.isoformat to use "Z" UTC designator
03fbca3 
Add an optional parameter to datetime.isoformat() and time.isoformat()
called use_utc_designator. If provided and True and the object is
associated with a tzinfo and its tzname is exactly "UTC", the formatted
UTC offset will be "Z" rather than an offset from UTC.
merwok
merwok reviewed Mar 22, 2022
View reviewed changes
Lib/datetime.py
tz = self._tzstr()
if tz:
s += tz
if use_utc_designator and "UTC" == self.tzname():
Copy link
Member

Choose a reason for hiding this comment

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

Minor thing: this is a strange style for a comparison in Python. The mistake of if something = oops_assigned_it doesn’t exist, so the natural order if self.tzname() == "UTC" can be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's fair. I'm not gonna polish this further until the datetime maintainers weigh in on whether they want to pursue this approach at all, but if they agree to this approach I'll make this change.

@godlygeek
Copy link
Contributor Author

Any thoughts, @pganssle? Would this resolve https://bugs.python.org/issue46614 to your satisfaction?

@pganssle
Copy link
Member

pganssle commented Apr 3, 2022

@godlygeek Sorry for the delay. I've left some more bikeshedd-y comments on https://bugs.python.org/issue46614, but I think broadly speaking this is probably the right approach.

A few thoughts in addition to the bikeshedding:

  1. Should we condition the Z designator on both a +00:00 output and tzname() == "UTC"? From ISO 8601's perspective, Z == +00:00, so we probably don't want to break that connection (even if you'd have to basically be deliberately crafting bad time zones to have this happen).
  2. Assuming the majority of people using UTC will be using timezone.utc, it might make sense to have a short circuit if dt.tzinfo is timezone.utc and not get the overhead of either calls to tzname or utcoffset. Probably increasingly people will also start calling with zoneinfo.ZoneInfo("UTC"), but I'm not sure I recommend that and ZoneInfo is pretty fast anyway.

@godlygeek
Copy link
Contributor Author

1. Should we condition the Z designator on both a +00:00 output and tzname() == "UTC"?

Possibly, though I don't think it's necessary, since as you say,

you'd have to basically be deliberately crafting bad time zones to have this happen

I'm inclined to think that if someone creates a timezone that self-reports its name as "UTC" but has a non-zero offset from UTC, having it show up as "Z" in .isoformat() would be one of the less surprising things about how it behaves. It's an easy change to make if you think it's worth the trouble and the extra call, though, and it wouldn't hurt to guarantee the consistency.

2. Assuming the majority of people using UTC will be using timezone.utc, it might make sense to have a short circuit

Since this will be opt-in rather than a default behavior, I wouldn't worry much about the performance implications of a single extra method call. This seems like a premature optimization to me, but it wouldn't be tough to implement if you think it's justified.

@MaxwellDupre
Copy link
Contributor

I recompiled from scratch and when I run datetimetester.py I get:

test_gaps (Lib.test.datetimetester.ZoneInfoTest[Africa/Harare]) ... ok
test_system_transitions (Lib.test.datetimetester.ZoneInfoTest[Africa/Harare]) ... skipped 'time module has no attribute tzset'

ERROR: test_divide_and_round (Lib.test.datetimetester.TestModule)

Traceback (most recent call last):
File "/home/dougal/Documents/GitHub/cpython/Lib/test/datetimetester.py", line 91, in test_divide_and_round
dar = datetime_module._divide_and_round
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: module 'datetime' has no attribute '_divide_and_round'

FAIL: test_compat_unpickle (Lib.test.datetimetester.TestDateTimeTZ)

Traceback (most recent call last):
File "/home/dougal/Documents/GitHub/cpython/Lib/test/datetimetester.py", line 4248, in test_compat_unpickle
self.assertIsInstance(derived.tzinfo, PicklableFixedOffset)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: cookie is not an instance of <class 'Lib.test.datetimetester.PicklableFixedOffset'>

FAIL: test_compat_unpickle (Lib.test.datetimetester.TestTimeTZ)

Traceback (most recent call last):
File "/home/dougal/Documents/GitHub/cpython/Lib/test/datetimetester.py", line 3865, in test_compat_unpickle
self.assertIsInstance(derived.tzinfo, PicklableFixedOffset)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: cookie is not an instance of <class 'Lib.test.datetimetester.PicklableFixedOffset'>

Ran 1754 tests in 8.046s
FAILED (failures=2, errors=1, skipped=429)

I cant say whether this is due to your changes or not especially when I see
'datetime' has no attribute '_divide_and_round'. Something odd going on here!
Makes me think am I testing this right?
Im running Fedora 35.

@pganssle pganssle mentioned this pull request May 3, 2022
Open
mguaypaq added a commit to neuropoly/changelog that referenced this pull request Nov 30, 2022
@mguaypaq
Add --use-milestone-due-date option
73b30b3 
See this CPython pull request and the linked discussion for an
explanation of why `replace('Z', '+00:00')` is needed:
python/cpython#32041
@joshuacwnewton joshuacwnewton mentioned this pull request Dec 21, 2022
Merged
4 tasks
@jasondamour
Copy link

@merwok @godlygeek Any progress on this? Would love to help if needed :D

@jasondamour jasondamour mentioned this pull request Aug 8, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@merwok merwok merwok left review comments

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

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

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

Successfully merging this pull request may close these issues.

None yet
8 participants
@godlygeek @pganssle @MaxwellDupre @jasondamour @merwok @the-knights-who-say-ni @ezio-melotti @bedevere-bot