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#shift for changing a time instance by calendar units #6598

Merged

Conversation

straight-shoota
Copy link
Member

@straight-shoota straight-shoota commented Aug 24, 2018

Based on #6574, only the last two commits are specific to this PR.

The main part of this PR renames Time#add_months to Time#shift and adds arguments for additional calendrical units.
This is copied from Timex.shift/2. An also fitting alternative would be #advance (from Rails' Time.advance).
This method can be used for adding arbitrary amounts of (conceptual) time units and forms a basis for an upcoming Time::DateSpan type (see #6522).

This is partly discussed in #6522 and #4556

Regarding performance, the new #shift is about twice as fast as #add_months for shifting by months/years. The comparison is not really fair though, because #add_months called year, month and day individually which means the rather expensive calculations in year_month_day_day_year are executed three times instead of once. With this simple optimization applied, #shift is still 50% faster than #add_months (probably because it does't go through the calendrical constructor).

Time#add_span is also renamed to Time#shift which is a more fitting name as well and means there are two implementations of Time#shift, one based on elapsed time in seconds, the other on calendrical units.
So now there are two ways for adding a time span to an instance of Time:
time + 2.hours advances by the duration of two hours on the instant time-line (elapsed time) and time.shift hours: 2 advances by two hours in local time (wall clock).

@bew
Copy link
Contributor

bew commented Aug 24, 2018

time + 2.hours advances by the duration of two hours on the instant time-line (elapsed time) and time.shift hours: 2 advances by two hours in local time (wall clock)

I don't understand the difference between the 2 methods (if there is one), could you give an example showing the difference? Ty

@straight-shoota
Copy link
Member Author

straight-shoota commented Aug 25, 2018

If the wall clock advances by two hours, it shows a time two hours later, but that doesn't mean the elapsed time between both instants also equals to two hours. For example when there is a daylight savings switch in between, they might be apart on the instant time line by the duration of only one hour or even three hours.

@robacarp
Copy link
Contributor

@straight-shoota first off, let me say thank you for tackling so much of the nonsense of time and date management. This is one of the things that can create a quality legacy in a language/stdlib and it can be so helpful.

I believe you're trying to capture the essence of what it means to modify a timestamp by measured time, instead of simply incrementing the digits timestamp as if it was simple math, which I believe is what time + delta does.

If that is the case, the word shift is opaque to me. I think I prefer advance, but in the case where a program is "advancing backwards", I'm less certain.

Either way, thanks for this.

@RX14 RX14 requested a review from bcardiff August 29, 2018 14:28
@asterite
Copy link
Member

@straight-shoota needs a rebase

@luislavena
Copy link
Contributor

Had a joke/suggestion to rename it to Time#travel instead 🤣

@straight-shoota
Copy link
Member Author

@robacarp Yes, Time#advance is a potential alternative. I think I prefer #shift because it fit's with either direction.

@straight-shoota
Copy link
Member Author

@luislavena I think the time travel metaphor fits better for manipulating the clock (such as timecop.cr).

@RX14 RX14 requested review from bcardiff and removed request for bcardiff September 5, 2018 11:19
@RX14
Copy link
Contributor

RX14 commented Sep 13, 2018

bump

@straight-shoota
Copy link
Member Author

ping @bcardiff

@straight-shoota straight-shoota force-pushed the jm/feature/time-shift branch 2 times, most recently from ed26a0e to c636933 Compare December 6, 2018 18:20
@straight-shoota straight-shoota added this to the 0.28.0 milestone Dec 31, 2018
@straight-shoota
Copy link
Member Author

Rebased and ready to ship.

@straight-shoota straight-shoota merged commit 9bcfeba into crystal-lang:master Feb 11, 2019
@straight-shoota straight-shoota deleted the jm/feature/time-shift branch February 11, 2019 14:17
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.

7 participants