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.

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

Add Time#to_local_in #6572

Merged
merged 1 commit into from Sep 10, 2018

Conversation

straight-shoota
Copy link
Member

Closes #5789

src/time.cr Outdated
# ```
# time = Time.now(Time::Location.new(-3600))
# time2 = Time.from_wall_clock(time, Time::Location.new(14400))
# time2.to_s("%Y-%m-%d %H:%M:%S") == time1.to_s("%Y-%m-%d %H:%M:%S") # => true
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: time1

@asterite
Copy link
Member

This looks a bit redundant: Time.from_wall_clock(time, ...). It reminds me of elixir: String.length(string).

Can we somehow provide an instance method instead?

@straight-shoota
Copy link
Member Author

It would of course also work as an instance method.
However, I purposely chose a constructor because it creates a new instance from combining the wall readings of the time argument with the given location.

This way it is not likely to be confused with Time#in(location).

I'm definitely open to change it to an instance method. Suggestions are welcome!

@asterite
Copy link
Member

time.to_wall_clock_in(location)

I'm not sure it reads well (grammatically), so if we can't come up with an instance name the class method is fine.

@jhass
Copy link
Member

jhass commented Aug 20, 2018

I'm fine with the class method but could also live with something like same_wall_clock_in.

src/time.cr Outdated
# ```
# time = Time.now
# time2 = Time.new(time.year, time.month, time.day, time.hour, time.minute, time.second, nanosecond: time.nanosecond, location: location)
# ```
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be helpful to talk about how this differs from Time#in. Right now it is difficult to tell which one to use and how they are different

src/time.cr Outdated
# Create a new `Time` instance that preserves the same wall clock reading than
# *time* but in a different *location*.
# Create a new `Time` instance that preserves the same local date-time
# representation (wall clock) as *time* but observed in a different *location*.
#
# ```
# time = Time.now(Time::Location.new(-3600))
# time2 = Time.from_wall_clock(time, Time::Location.new(14400))
Copy link
Member

@jhass jhass Aug 20, 2018

Choose a reason for hiding this comment

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

Maybe we can use two real timezones like New York and Tokyo and then call the vars new_york_time, tokyo_wall_clock_time or so? (Admittedly not great names yet). time, time2 gets confusing fast here :/

Copy link
Contributor

@RX14 RX14 left a comment

Choose a reason for hiding this comment

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

Needs to not be a class method.

@bcardiff
Copy link
Member

If Time is seen as an immutable struct, like record maybe we can keep the recent idiom of #copy_with method that will allow the user to override any specific part of the value.

@straight-shoota
Copy link
Member Author

straight-shoota commented Aug 21, 2018

@bcardiff copy_with might be a useful addition to Time, but I'm not sure it would be a great here, because it doesn't tell there's a difference to #in. In fact, I would expect #copy_with with a location to do the same thing as #in because it just copies the instance with the bew location.

@straight-shoota
Copy link
Member Author

straight-shoota commented Aug 21, 2018

If there was a local date time representation (floating time), this could be expressed for example like in the Java 8 Date Time API as time.toLocal().atZone(location).
Without the intermediary step, this could become time.to_local_in(location) in Crystal. That's similar to @asterite proposal but with a more formal term.

@straight-shoota
Copy link
Member Author

straight-shoota commented Sep 4, 2018

Rebased and renamed Time.from_wall_clock to Time#to_local_in.

@straight-shoota straight-shoota changed the title Add Time.from_wall_clock Add Time#to_local_in Sep 5, 2018
src/time.cr Outdated
# ```
# tokyo = Time.now(Time::Location.load("Asia/Tokyo"))
# new_york = Time.new(tokyo.year, tokyo.month, tokyo.day, tokyo.hour, tokyo.minute, tokyo.second, nanosecond: tokyo.nanosecond, location: location)
# ```
Copy link
Contributor

@ysbaddaden ysbaddaden Sep 5, 2018

Choose a reason for hiding this comment

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

This is too abstract. The naming doesn't give much hint at what the method means to achieve, and the documentation doesn't really help until the last example.

The following would be more helpful, and enough to understand:

Creates a new Time instance with only the location changed. Unlike #in that will shift the date time so the new Time instance still refers to the same UTC timestamp, #to_local_in keeps the same date and time but for another location.

For example:

halloween = Time.utc(2018, 10, 31, 17, 30, 0)
tokyo = halloween.to_local_in(Time::Location.load("Asia/Tokyo"))
new_york = halloween.to_local_in(Time::Location.load("America/New_York"))
tokyo.to_s    # => 2018-10-31 17:30:00 Asia/Tokyo
new_york.to_s # => 2018-09-31 17:30:00 America/New_York

Copy link
Member Author

Choose a reason for hiding this comment

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

Creates a new Time instance with only the location changed.

This reduces the time instance to it's local date-time representation, but in fact, this method also changes the instant on the time-line.

But I'll try to improve the documentation.

src/time.cr Outdated
#
# The result of this method is equivalent to going through the calendrical
# constructor like this:
# Unlike `#in`, which always references the same instant in time, `#to_local_in`
Copy link
Contributor

Choose a reason for hiding this comment

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

which always preserves the same instant in time

@straight-shoota
Copy link
Member Author

Let me squash this before merging 👍

@RX14 RX14 merged commit 0b92864 into crystal-lang:master Sep 10, 2018
@RX14 RX14 added this to the 0.27.0 milestone Sep 10, 2018
@straight-shoota straight-shoota deleted the jm/feature/time-same-wall branch September 10, 2018 19:23
ezrast pushed a commit to ezrast/crystal that referenced this pull request Oct 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants