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

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
@adzap
Contributor

adzap commented Aug 7, 2011

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

This comment has been minimized.

Show comment
Hide comment

nurous commented Aug 9, 2011

+1

@isaacsanders

This comment has been minimized.

Show comment
Hide comment
@isaacsanders

isaacsanders Apr 28, 2012

Contributor

@adzap Is this still an issue?

Contributor

isaacsanders commented Apr 28, 2012

@adzap Is this still an issue?

@adzap

This comment has been minimized.

Show comment
Hide comment
@adzap

adzap Apr 29, 2012

Contributor

I'll need to verify.

Contributor

adzap commented Apr 29, 2012

I'll need to verify.

@adzap

This comment has been minimized.

Show comment
Hide comment
@adzap

adzap Apr 29, 2012

Contributor

Yes, it is still an issue.

Contributor

adzap commented Apr 29, 2012

Yes, it is still an issue.

@isaacsanders

This comment has been minimized.

Show comment
Hide comment
@isaacsanders

isaacsanders Apr 29, 2012

Contributor

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

Contributor

isaacsanders commented Apr 29, 2012

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

This comment has been minimized.

Show comment
Hide comment
@courtland

courtland May 1, 2012

Contributor

+1

Contributor

courtland commented May 1, 2012

+1

@courtland

This comment has been minimized.

Show comment
Hide comment
@courtland

courtland May 1, 2012

Contributor

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

Contributor

courtland commented May 1, 2012

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

This comment has been minimized.

Show comment
Hide comment
@adzap

adzap May 1, 2012

Contributor

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.

Contributor

adzap commented May 1, 2012

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 Aug 7, 2011

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.
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.
@adzap

This comment has been minimized.

Show comment
Hide comment
@adzap

adzap May 1, 2012

Contributor

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

Contributor

adzap commented May 1, 2012

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

+ 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

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva May 2, 2012

Member

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

@carlosantoniodasilva

carlosantoniodasilva May 2, 2012

Member

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

@adzap

This comment has been minimized.

Show comment
Hide comment
@adzap

adzap May 2, 2012

Contributor

Should be ready to go now.

Contributor

adzap commented May 2, 2012

Should be ready to go now.

@isaacsanders

This comment has been minimized.

Show comment
Hide comment
@isaacsanders

isaacsanders May 3, 2012

Contributor

@carlosantoniodasilva Is it good for merging?

Contributor

isaacsanders commented May 3, 2012

@carlosantoniodasilva Is it good for merging?

@carlosantoniodasilva

This comment has been minimized.

Show comment
Hide comment
@carlosantoniodasilva

carlosantoniodasilva May 3, 2012

Member

I believe it is, thanks.

@pixeltrix could you please review this?

I believe it is, thanks.

@pixeltrix could you please review this?

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix May 3, 2012

Member

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.

Member

pixeltrix commented May 3, 2012

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

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix May 3, 2012

Member

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

Member

pixeltrix commented May 3, 2012

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

@adzap

This comment has been minimized.

Show comment
Hide comment
@adzap

adzap May 3, 2012

Contributor

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.

Contributor

adzap commented May 3, 2012

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.

- assert_equal Date.new(2005, 2, 21), Date.new(2005, 2, 21).to_date
+ date = Date.new(2005, 2, 21)
+
+ assert date.equal?(date)

This comment has been minimized.

@pixeltrix

pixeltrix May 3, 2012

Member

Missing to_date here

@pixeltrix

pixeltrix May 3, 2012

Member

Missing to_date here

This comment has been minimized.

@adzap

adzap May 3, 2012

Contributor

Right you are. Good find.

@adzap

adzap May 3, 2012

Contributor

Right you are. Good find.

@adzap

This comment has been minimized.

Show comment
Hide comment
@adzap

adzap May 3, 2012

Contributor

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.

Contributor

adzap commented May 3, 2012

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

This comment has been minimized.

Show comment
Hide comment
@adzap

adzap May 3, 2012

Contributor

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

Contributor

adzap commented May 3, 2012

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

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

This comment has been minimized.

@pixeltrix

pixeltrix May 3, 2012

Member

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

@pixeltrix

pixeltrix May 3, 2012

Member

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

This comment has been minimized.

@adzap

adzap May 3, 2012

Contributor

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
@adzap

adzap May 3, 2012

Contributor

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

This comment has been minimized.

@pixeltrix

pixeltrix May 3, 2012

Member

True - why not use assert_same?

@pixeltrix

pixeltrix May 3, 2012

Member

True - why not use assert_same?

This comment has been minimized.

@adzap

adzap May 3, 2012

Contributor

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

@adzap

adzap May 3, 2012

Contributor

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

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix May 3, 2012

Member

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.

Member

pixeltrix commented May 3, 2012

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

This comment has been minimized.

Show comment
Hide comment
@adzap

adzap May 3, 2012

Contributor

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

Contributor

adzap commented May 3, 2012

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

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix May 3, 2012

Member

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

Member

pixeltrix commented May 3, 2012

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

@adzap

This comment has been minimized.

Show comment
Hide comment
@adzap

adzap May 4, 2012

Contributor

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

Contributor

adzap commented May 4, 2012

@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

This comment has been minimized.

Show comment
Hide comment
@phene

phene May 31, 2012

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

phene commented May 31, 2012

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

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca May 31, 2012

Member

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

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

Member

rafaelfranca commented May 31, 2012

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

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

@ghost ghost assigned pixeltrix May 31, 2012

@adzap

This comment has been minimized.

Show comment
Hide comment
@adzap

adzap May 31, 2012

Contributor

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.

Contributor

adzap commented May 31, 2012

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

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix Jun 1, 2012

Member

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

Member

pixeltrix commented Jun 1, 2012

@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

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Sep 26, 2012

Member

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?

Member

schneems commented Sep 26, 2012

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

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Nov 18, 2012

Member

Two month ping. @pixeltrix , whatcha think?

Member

steveklabnik commented Nov 18, 2012

Two month ping. @pixeltrix , whatcha think?

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix Nov 20, 2012

Member

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.

Member

pixeltrix commented Nov 20, 2012

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 in b79adc4 Jan 21, 2013

korny referenced this pull request Mar 12, 2013

refactored Time#<=> and DateTime#<=> by removing unnecessary calls wi…
…thout losing performance

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

This comment has been minimized.

Show comment
Hide comment
@likethesky

likethesky May 24, 2013

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.

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

This comment has been minimized.

Show comment
Hide comment
@vipulnsward

vipulnsward May 24, 2013

Member

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

Member

vipulnsward commented May 24, 2013

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

@likethesky

This comment has been minimized.

Show comment
Hide comment
@likethesky

likethesky May 24, 2013

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.

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