Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #3722 +/- ##
=======================================
Coverage 84.28% 84.28%
=======================================
Files 133 133
Lines 14261 14261
Branches 2405 2405
=======================================
Hits 12020 12020
Misses 1490 1490
Partials 751 751 |
| ( | ||
| "2021-01-01T00:00:00.000000", | ||
| datetime(2021, 1, 1, tzinfo=timezone.utc), | ||
| ), # No TZ -- assume UTC | ||
| datetime(2021, 1, 1).astimezone(timezone.utc), | ||
| ), # No TZ -- assume local but convert to UTC |
There was a problem hiding this comment.
given the comment I actually think the implementation is incorrect and should be something like:
try:
dt = datetime.fromisoformat(s)
except ...:
...
else:
if dt.tzinfo is None:
return dt.replace(tzinfo=UTC)
else:
return dt.astimezone(UTC)There was a problem hiding this comment.
I wrote that comment 😅 And after reading the ISO 8601 spec, I think I was mistaken:
https://en.wikipedia.org/wiki/ISO_8601#Local_time_(unqualified)
If no UTC relation information is given with a time representation, the time is assumed to be in local time.
Now there's the obvious warning this being unsafe in the next sentence but I'm split between being spec-compliant or "doing the right thing" which may confuse people if they rely on the spec.
What's your take @asottile-sentry?
There was a problem hiding this comment.
I suspect it won't matter as most should run their servers in UTC anyway -- so I guess we can go with the current implementation
it seems to only be used to sort breadcrumbs in the event payload (is that even necessary? surely relay or the frontend should sort those right? do we even need this function and the caller?)
There was a problem hiding this comment.
surely relay or the frontend should sort those right?
Agree with this but maybe we're trying to save resources? It also seems like the side effect of converting a string to a python date might be something we are after here. That said that python date will then be converted back into an ISO 8601 timestamp so 🤷🏻
Will ask ingest folks.

Fixes #3720.