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

Update ZonedDateTime constructor to clarify behaviour when from_utc=false #367

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

Update ZonedDateTime constructor to clarify behaviour when from_utc=false #367

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/types/zoneddatetime.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,10 @@ end
ZonedDateTime(dt::DateTime, tz::TimeZone; from_utc=false) -> ZonedDateTime

Construct a `ZonedDateTime` by applying a `TimeZone` to a `DateTime`. When the `from_utc`
keyword is true the given `DateTime` is assumed to be in UTC instead of in local time and is
converted to the specified `TimeZone`. Note that when `from_utc` is true the given
`DateTime` will always exists and is never ambiguous.
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.
Comment on lines +31 to +34
Copy link
Member

@omus omus Nov 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Better?

Suggested change
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.

Copy link
Member Author

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.

Copy link
Member

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 :)

"""
function ZonedDateTime(dt::DateTime, tz::VariableTimeZone; from_utc::Bool=false)
# Note: Using a function barrier which reduces allocations
Expand Down