-
-
Notifications
You must be signed in to change notification settings - Fork 238
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
Merged
+20
−4
Merged
Changes from 1 commit
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next
Next commit
TimeStamper() now uses TZ-aware objects
The default output doesn't change but manual formatting allows for TZ data now. Fixes #703
commit 7022f44c5d13f46ccb44ca49873763255ebb9ac6
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.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: 9e0e47eThere 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