Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fix TimeWithZone#to_time to return self instead of utc #2453

Closed
wants to merge 6 commits into from
@adzap

A previous change to TimeWithZone#to_time returned utc. This is unexpected and doesn't match the comment above the method. The original change was made to simplify the <=> override on Time. Using a different technique achieves the same result while preserving the expected to_time behaviour.

Also added better tests for the temporal classes when they should return self i.e. Time#to_time, Date#to_date and DateTime#to_datetime.

Inexplicably Ruby 1.9 does not return self from Time#to_time, but getlocal instead. I wonder if we should override this?

@nurous

+1

@isaacsanders

@adzap Is this still an issue?

@adzap

I'll need to verify.

@adzap

Yes, it is still an issue.

@isaacsanders

Is there any reason not to merge? @adzap, have you rebased onto the current state of master? It would be easier to merge this if you did. cc/ @josevalim

@courtland


1.9.3p125 :043 > Rails.version
=> "3.2.3"
1.9.3p125 :044 > t = SharedCredentialGroup.last.starts_at
SharedCredentialGroup Load (0.5ms) SELECT "shared_credential_groups".* FROM "shared_credential_groups" ORDER BY "shared_credential_groups"."id" DESC LIMIT 1
=> Tue, 01 May 2012 11:00:28 EDT -04:00
1.9.3p125 :045 > t.inspect
=> "Tue, 01 May 2012 11:00:28 EDT -04:00"
1.9.3p125 :046 > t.class
=> ActiveSupport::TimeWithZone
1.9.3p125 :047 > t.to_time
=> 2012-05-01 15:00:28 UTC

@adzap

I thought you could edit the commits in a open pull request. Apparently not. I did rebase with push force on my branch which has not worked as I expected.

Pushing new commits to tweak a pull request can end up very noisy. I will start a new pull request and do his again and link to it here.

adzap added some commits
@adzap adzap Fix TimeWithZone#to_time to return self
A previous commit (7872cc9) changed it to return utc to simplify the Time#<==>
override. An alternative method acheives the same result while
preserving original behaviour.
0784a7c
@adzap adzap Better tests for Time#to_time, Date#to_date and DateTime#to_datetime
These should all return self, except for Time#to_time on Ruby 1.9, which
returns utc as the native method which is not overridden by AS but
perhaps should be.
222228e
@adzap

Oh well that fixed it. This should now apply cleanly.

activesupport/test/core_ext/time_ext_test.rb
@@ -587,8 +587,15 @@ def test_to_datetime
assert_equal ::Date::ITALY, Time.utc(2005, 2, 21, 17, 44, 30).to_datetime.start # use Ruby's default start value
end
- def test_to_time
- assert_equal Time.local(2005, 2, 21, 17, 44, 30), Time.local(2005, 2, 21, 17, 44, 30).to_time
+ if RUBY_VERSION < '1.9'
+ def test_to_time
+ time = Time.local(2005, 2, 21, 17, 44, 30)
+ assert time.equal?(time.to_time)
+ end
+ else
+ def test_to_time
+ assert_equal Time.local(2005, 2, 21, 17, 44, 30), Time.local(2005, 2, 21, 17, 44, 30).to_time
+ end

I believe there's no need to check for ruby version, now that master aims 1.9 only :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@adzap

Should be ready to go now.

@isaacsanders

@carlosantoniodasilva Is it good for merging?

@carlosantoniodasilva

I believe it is, thanks.

@pixeltrix could you please review this?

@pixeltrix
Owner

The original commit that changes to_time to return UTC doesn't have any tests so we need to check we're not breaking something.

@pixeltrix
Owner

@adzap where does Ruby 1.9 return UTC from to_time - my tests seem to indicate otherwise.

@adzap

You're right. I don't know where I got this idea from. Possibly older version of 1.9.2? Or I was completely wrong.

activesupport/test/core_ext/date_ext_test.rb
@@ -36,7 +36,9 @@ def test_to_datetime
end
def test_to_date
- assert_equal Date.new(2005, 2, 21), Date.new(2005, 2, 21).to_date
+ date = Date.new(2005, 2, 21)
+
+ assert date.equal?(date)
@pixeltrix Owner

Missing to_date here

@adzap
adzap added a note

Right you are. Good find.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@adzap

I found the source of confusion. Time#to_time will return Time#getlocal in Ruby 1.9.3 and 1.9.2, instead of self.

@adzap

I've updated the pull request initial message to reflect this.

activesupport/test/core_ext/date_ext_test.rb
@@ -36,7 +36,9 @@ def test_to_datetime
end
def test_to_date
- assert_equal Date.new(2005, 2, 21), Date.new(2005, 2, 21).to_date
+ date = Date.new(2005, 2, 21)
+
+ assert date.equal?(date.to_date)
@pixeltrix Owner

I'd prefer to keep these as assert_equal - the error message is better.

@adzap
adzap added a note

But they are not semantically equivalent. I am trying demonstrate that the method returns self. If you use assert_equal, then only the value equality is verified. This is not so much an issue for Dates, but Times you get

assert_equal time.utc, time # passes
@pixeltrix Owner

True - why not use assert_same?

@adzap
adzap added a note

Didn't know it existed. I spend my days in RSpec. Will update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pixeltrix
Owner

When you're done changing the tests back to assert_equal can you squash the commits. Once that's done I'll merge it.

Regarding Ruby's Time.to_time the general rule is that we provide an alternative, e.g: Time.current vs. Time.now. Obviously we can't apply that rule here exactly but I'd lean on the side of not altering the behaviour of core methods.

@adzap

While I sympathise, you have a mismatch between Time and TimeWithZone, which is meant to act as drop in replacement for the most part.

@pixeltrix
Owner

Which makes me think that we should be returning getlocal rather than self.

@adzap

@pixeltrix I'm inclined to agree, sadly. How the hell did Ruby core mess that up. An ActiveSupport-ism adopted, but implemented incorrectly. Sigh.

I'd be happy to seek the council of other Rails core members.

@phene

Any chance of this getting pulled in? My team just discovered this when regression testing a Rails 3.0 to 3.1 upgrade.

@rafaelfranca

@phene this can be pulled but I think that is only for master.

@pixeltrix I'm assign this to you ;).

@pixeltrix pixeltrix was assigned
@adzap

The outstanding issue is that, if this is pulled, TimeWithZone#to_time and Time#to_time will behave differently, due 1.9's crazy behaviour of returning getlocal.

Consistency across these is required. But that requires AS changing the core Time class, or TWZ doing the annoying thing of returning getlocal. I really think that Ruby core should fix it and back port, unless they had a valid reasoning.

@pixeltrix
Owner

@adzap I'm leaning towards TimeWithZone#to_time returning getlocal as that's what all of 1.9's own to_time methods return for Date, DateTime and Time. I'm looking at all of the date & time code as there's a lot of stuff that's based on 1.8's limitations - for example our DateTime#to_time can be removed and we don't need to convert to UTC to compare times as they work fine with <=> and different offsets.

@schneems
Collaborator

4 months, what is the status on this? Seems like we are leaning towards not fixing this to preserve compatibility? Is it still up for consideration or can we close?

@steveklabnik
Collaborator

Two month ping. @pixeltrix , whatcha think?

@pixeltrix
Owner

I've a bunch of time related commits I need to rebase and merge including this one:

Make to_time consistent across implementations

Currently to_time behaves differently depending on whether self is a
Time, ActiveSupport::TimeWithZone, String, Date or DateTime. The native
Ruby implementations all return an instance of Time in the local time
zone so this commit changes the various implementations to match that.

The one possible source of breakages is the changing of String#to_time
to return a local time rather than a UTC time by default. Research seems
to show that use of this method is low so any likely impact is minimal.

Closes #2453

so leave it open for now, thanks.

@pixeltrix pixeltrix closed this pull request from a commit
@pixeltrix pixeltrix Standardise the return value of `to_time`
This commit standardises the return value of `to_time` to an instance
of `Time` in the local system timezone, matching the Ruby core and
standard library behavior.

The default form for `String#to_time` has been changed from :utc to
:local but research seems to suggest the latter is the more common form.

Also fix an edge condition with `String#to_time` where the string has
a timezone offset in it and the mode is :local. e.g:

  # Before:
  >> "2000-01-01 00:00:00 -0500".to_time(:local)
  => 2000-01-01 05:00:00 -0500

  # After:
  >> "2000-01-01 00:00:00 -0500".to_time(:local)
  => 2000-01-01 00:00:00 -0500

Closes #2453
b79adc4
@pixeltrix pixeltrix closed this in b79adc4
@korny korny referenced this pull request from a commit
@dcrec1 dcrec1 refactored Time#<=> and DateTime#<=> by removing unnecessary calls wi…
…thout losing performance

Signed-off-by: Santiago Pastorino <santiago@wyeworks.com>
7872cc9
@likethesky

Perhaps I should start a new issue (if so, let me know and I will), but @pixeltrix the following now breaks in Rails 3.2.x running with Ruby 1.9.3-p429: jruby/jruby#755 (comment) Not sure if this affects Rails 4 as well, or where I should go to report this. This issue looked "closest" to the misbehavior we're seeing.

@vipulnsward

@likethesky I have created a fix for this at #10686
That should fix it.

@likethesky

Thanks @vipulnsward ... I made a comment there asking if this will be backported to other Rails versions. I also confirmed that your fix does indeed fix our issue in both MRI (p429) and in JRuby (on 1.9.3-p392 [jruby 1.7.4], which surprisingly doesn't fail on MRI p392, even though it does on MRI p429!). Cheers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on May 1, 2012
  1. @adzap

    Fix TimeWithZone#to_time to return self

    adzap authored
    A previous commit (7872cc9) changed it to return utc to simplify the Time#<==>
    override. An alternative method acheives the same result while
    preserving original behaviour.
  2. @adzap

    Better tests for Time#to_time, Date#to_date and DateTime#to_datetime

    adzap authored
    These should all return self, except for Time#to_time on Ruby 1.9, which
    returns utc as the native method which is not overridden by AS but
    perhaps should be.
Commits on May 2, 2012
  1. @adzap
Commits on May 3, 2012
  1. @adzap
Commits on May 6, 2012
  1. @adzap
  2. @adzap
This page is out of date. Refresh to see the latest.
View
3  activesupport/lib/active_support/core_ext/time/calculations.rb
@@ -379,9 +379,10 @@ def minus_with_coercion(other)
# Layers additional behavior on Time#<=> so that DateTime and ActiveSupport::TimeWithZone instances
# can be chronologically compared with a Time
def compare_with_coercion(other)
+ other = other.comparable_time if other.respond_to?(:comparable_time)
# we're avoiding Time#to_datetime cause it's expensive
if other.is_a?(Time)
- compare_without_coercion(other.to_time)
+ compare_without_coercion(other)
else
to_datetime <=> other
end
View
2  activesupport/lib/active_support/time_with_zone.rb
@@ -279,7 +279,7 @@ def to_i
# A TimeWithZone acts like a Time, so just return +self+.
def to_time
- utc
+ self
end
def to_datetime
View
3  activesupport/test/core_ext/date_ext_test.rb
@@ -36,7 +36,8 @@ def test_to_datetime
end
def test_to_date
- assert_equal Date.new(2005, 2, 21), Date.new(2005, 2, 21).to_date
+ date = Date.new(2005, 2, 21)
+ assert_same date.to_date, date
end
def test_change
View
4 activesupport/test/core_ext/date_time_ext_test.rb
@@ -30,7 +30,9 @@ def test_to_date
end
def test_to_datetime
- assert_equal DateTime.new(2005, 2, 21, 14, 30, 0), DateTime.new(2005, 2, 21, 14, 30, 0).to_datetime
+ datetime = DateTime.new(2005, 2, 21, 14, 30, 0)
+
+ assert datetime.to_datetime, datetime
end
def test_to_time
View
8 activesupport/test/core_ext/time_ext_test.rb
@@ -588,7 +588,13 @@ def test_to_datetime
end
def test_to_time
- assert_equal Time.local(2005, 2, 21, 17, 44, 30), Time.local(2005, 2, 21, 17, 44, 30).to_time
+ with_env_tz 'US/Eastern' do
+ # Ruby 1.9 Time#to_time returns getlocal, which deviates from the original
+ # AS implementation returning self. The core Time class method is unchanged.
+ time = Time.utc(2005, 2, 21, 17, 44, 30)
+ # Compare as strings with values and zone
+ assert_equal time.to_time.to_s(:rfc822), time.getlocal.to_s(:rfc822)
+ end
end
# NOTE: this test seems to fail (changeset 1958) only on certain platforms,
View
2  activesupport/test/core_ext/time_with_zone_test.rb
@@ -319,7 +319,7 @@ def test_to_i_with_wrapped_datetime
end
def test_to_time
- assert_equal @twz, @twz.to_time
+ assert_same @twz.to_time, @twz
end
def test_to_date
Something went wrong with that request. Please try again.