-
Notifications
You must be signed in to change notification settings - Fork 54
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
Update ZonedDateTime constructor to clarify behaviour when from_utc=false #367
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #367 +/- ##
=======================================
Coverage 93.75% 93.75%
=======================================
Files 31 31
Lines 1553 1553
=======================================
Hits 1456 1456
Misses 97 97
Continue to review full report at Codecov.
|
Definitely seems to be a perspective issue. The original documentation was written with the idea that it was known that internally a UTC datetime is stored. The primary reason |
keyword is true the given `DateTime` is assumed to be in UTC and is converted to the specified | ||
`TimeZone`. When `from_utc` is false the given `DateTime` is assumed to already be in the | ||
specified `TimeZone`. Note that when `from_utc` is true the given `DateTime` will always exist | ||
and is never ambiguous. |
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.
Better?
keyword is true the given `DateTime` is assumed to be in UTC and is converted to the specified | |
`TimeZone`. When `from_utc` is false the given `DateTime` is assumed to already be in the | |
specified `TimeZone`. Note that when `from_utc` is true the given `DateTime` will always exist | |
and is never ambiguous. | |
keyword is `true` the given `DateTime` is declared to be in UTC but will be displayed in the specified | |
`TimeZone`. When `from_utc` is `false` the given `DateTime` is declared to be in local time (the | |
specified `TimeZone`). Note that when `from_utc` is `true` the provided UTC `DateTime` will always exist | |
and is never ambiguous. |
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.
I think "will be displayed" leaks the abstraction; to the user, they don't need to know that the underlying storage is in UTC. I can see why you would want to avoid "converted" though. I'm struggling to come up with good wording.
I think the phrase "local time" should be avoided to avoid confusion with localzone()
/the OS time zone.
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.
I think "will be displayed" leaks the abstraction; to the user, they don't need to know that the underlying storage is in UTC. I can see why you would want to avoid "converted" though. I'm struggling to come up with good wording.
I'm definitely up for suggestions. I think this part of the documentation has been hard to get right.
I think the phrase "local time" should be avoided to avoid confusion with
localzone()
/the OS time zone.
The phrase "local time" is something the IANA documentation uses and isn't quite perfect as really it just refers to only the time component. Maybe "local timestamp" or "local DateTime
" is an improvement.
I suspect we'll have to iterate on this a little bit. Thank you for opening the PR :)
I got caught by this and almost assumed something incorrect about the downstream CloudWatchLogs.jl library. The "instead of in local time" clause implied to me that when
from_utc=false
theDateTime
would be converted fromlocalzone()
into the specifiedTimeZone
, but that is not the case.