-
-
Notifications
You must be signed in to change notification settings - Fork 237
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
TimeStamper() now uses TZ-aware objects #709
Conversation
The default output doesn't change but manual formatting allows for TZ data now. Fixes #703
src/structlog/processors.py
Outdated
event_dict[key] = now().isoformat() | ||
# We remove the timezone offset for backwards-compatibility. If the | ||
# user wants a timezone, they have to set fmt manually. | ||
event_dict[key] = now().isoformat().rsplit("+", 1)[0] |
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.
Counterexample:
>>> datetime.datetime.now().astimezone(datetime.timezone(datetime.timedelta(hours=-5))).isoformat().rsplit("+", 1)[0]
'2025-03-08T09:55:44.415778-05:00'
datetime.isoformat() states that its ISO time zone designator format is +HH:MM[:SS[.ffffff]]
, but right below they give us a sample with negative offset—'2009-11-27T00:00:00.000100-06:39'
.
I.e. their +
in +HH:MM
is actually "plus or minus". Negative offsets are totally valid by ISO 8601 / RFC 3339 anyway.
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.
bleh true. great, according to the docstring, the TZ is always +zz:zz
so before we start imitating using strftime, just a [:-6]
?
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.
My proposal: datetime.datetime.now().astimezone().replace(tzinfo=None).isoformat()
,
i.e. event_dict[key] = now().replace(tzinfo=None).isoformat()
datetime.astimezone() reads: "If you merely want to remove the timezone object from an aware datetime dt without conversion of date and time data, use dt.replace(tzinfo=None)." I think it is exactly our case: we want to convert aware time object back to naive, i.e. remove tzinfo
.
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.
wait, this is stupid. with either approach, I'm adding and removing the timezone instead of adding it only when necessary. i.e. in stamper_fmt
. behold the genius who finally read his own code: 9e0e47e
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.
Well, isoformat() uses _tzstr() which in turn uses _format_offset(), which gives us sing and possibly seconds and microseconds.
I can't imagine real worlds applications for TZ offset with seconds and its fractions (some crazy astronomy stuff??), but is valid per current standards, so just [:-6]
is not enough because time zone designator has variable length.
Yes, docstrings for these functions are confusing indeed, they repeatedly use +hh:mm
which really is just one possible option...
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.
since we posted at almost the same time, check back my comment before. I think I solved it pretty much perfectly.
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.
Yes, I do agree. I also tested it right now and it works as intended. Thanks to both of us:D
The default output doesn't change but manual formatting allows for TZ data now.
Fixes #703
cc @andrei-korshikov