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

Projects
None yet
7 participants
@straight-shoota
Copy link
Member

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

@RX14

RX14 approved these changes Aug 24, 2018

@bew

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link

commented Aug 27, 2018

@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 Aug 29, 2018

@asterite

This comment has been minimized.

Copy link
Member

commented Aug 30, 2018

@straight-shoota needs a rebase

@luislavena

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2018

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

@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/time-shift branch from b5dc65c to e6a6d6c Sep 4, 2018

@straight-shoota

This comment has been minimized.

Copy link
Member Author

commented Sep 4, 2018

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

@straight-shoota

This comment has been minimized.

Copy link
Member Author

commented Sep 4, 2018

@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 Sep 5, 2018

@RX14

RX14 approved these changes Sep 5, 2018

@RX14

This comment has been minimized.

Copy link
Member

commented Sep 13, 2018

bump

@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/time-shift branch from e6a6d6c to 50a2da8 Sep 14, 2018

@straight-shoota

This comment has been minimized.

Copy link
Member Author

commented Oct 3, 2018

ping @bcardiff

@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/time-shift branch from 50a2da8 to 88b3a90 Oct 16, 2018

@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/time-shift branch 2 times, most recently from ed26a0e to c636933 Dec 5, 2018

@straight-shoota straight-shoota added this to the 0.28.0 milestone Dec 31, 2018

straight-shoota added some commits Aug 21, 2018

@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/time-shift branch from c636933 to 8686174 Feb 10, 2019

@straight-shoota

This comment has been minimized.

Copy link
Member Author

commented Feb 10, 2019

Rebased and ready to ship.

@straight-shoota straight-shoota merged commit 9bcfeba into crystal-lang:master Feb 11, 2019

5 checks passed

ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@straight-shoota straight-shoota deleted the straight-shoota:jm/feature/time-shift branch Feb 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.