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

Fix ActiveSupport::TimeWithZone#in #26597

Merged
merged 1 commit into from
Sep 24, 2016
Merged

Fix ActiveSupport::TimeWithZone#in #26597

merged 1 commit into from
Sep 24, 2016

Conversation

tbalthazar
Copy link
Contributor

Summary

ActiveSupport::TimeWithZone#in is now an alias for ActiveSupport::TimeWithZone#+ instead of being sent to a Time instance (via method_missing). It no longer returns wrong data when used on a TimeWithZone near DST.

Example wrong data that was returned before the fix:

Time.zone = ActiveSupport::TimeZone.new("America/New_York")
t = Time.zone.local(2016,11,6,1) # Sun, 06 Nov 2016 01:00:00 EDT -04:00
t + 1.hour   # ok => Sun, 06 Nov 2016 01:00:00 EST -05:00
t.in(1.hour) # ko => Sun, 06 Nov 2016 02:00:00 EST -05:00

It fixes #26580.

@rails-bot
Copy link

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @matthewd (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

@pixeltrix
Copy link
Contributor

pixeltrix commented Sep 24, 2016

@tbalthazar code is 👍 but can I just ask for a couple of things:

  1. Can you change the CHANGELOG entry to this:

    *   Fix `ActiveSupport::TimeWithZone#in` across DST boundaries.
    
    Previously calls to `in` were being sent to the non-DST aware
    method `Time#since` via `method_missing`. It is now aliased to
    the DST aware `ActiveSupport::TimeWithZone#+` which handles
    transitions across DST boundaries, e.g:
    
        Time.zone = "US/Eastern"
    
        t = Time.zone.local(2016,11,6,1)
        # => Sun, 06 Nov 2016 01:00:00 EDT -05:00 
    
        t.in(1.hour)
        # => Sun, 06 Nov 2016 01:00:00 EST -05:00 
    
    Fixes #26580.
    
    *Thomas Balthazar*
  2. Can you rebase and squash the merge commits

Thanks ❤️

Previously calls to `in` were being sent to the non-DST aware
method `Time#since` via `method_missing`. It is now aliased to
the DST aware `ActiveSupport::TimeWithZone#+` which handles
transitions across DST boundaries, e.g:

    Time.zone = "US/Eastern"

    t = Time.zone.local(2016,11,6,1)
    # => Sun, 06 Nov 2016 01:00:00 EDT -05:00

    t.in(1.hour)
    # => Sun, 06 Nov 2016 01:00:00 EST -05:00
@tbalthazar
Copy link
Contributor Author

@pixeltrix Great suggestion, your changelog entry is better written, thanks!
Looks like the build failed this time, but I'm pretty sure it's not related to this commit.
Could you please relaunch the Travis checks? (Or is there any way I can do that myself?)
Thanks!

@pixeltrix
Copy link
Contributor

@tbalthazar restarted the Travis jobs 👍

@pixeltrix pixeltrix merged commit bb3bc47 into rails:master Sep 24, 2016
@pixeltrix
Copy link
Contributor

Backported to 5-0-stable in 0a7176c and 4-2-stable in a6edbeb.

Slightly complicated by the fact that up until #18063 since was a method on AS::TWZ and given the (slight) breaking change by that PR it made sense to adjust the alias to since instead of +.

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.

Using .in(x.hours) on a TimeWithZone near DST boundary skips hours
5 participants