[PATCH] Fixing inconsistencies with Time{WithZone}#{hash,eql?} #905

Closed
lighthouse-import opened this Issue May 16, 2011 · 6 comments

Comments

Projects
None yet
1 participant

Imported from Lighthouse. Original ticket at: http://rails.lighthouseapp.com/projects/8994/tickets/6534
Created by Marc-André Lafortune - 2011-03-05 20:51:35 UTC

If foo.eql?(bar) returns true then so should bar.eql?(foo).
Moreover, foo.hash must then be the same as bar.hash.
Otherwise the Hash lookup will fail. TimeWithZone want to act like a Time, but currently:

bar = Time.utc(2000)
foo = bar.in_time_zone

foo.eql?(bar) # => true
bar.eql?(foo) # => false
foo.hash == bar.hash # => false (only in Ruby 1.9)

The attached patch fixes this. Since TimeWithZone wants to be freely replaceable with a Time:

bar.eql?(foo) # => now true
foo.hash == bar.hash # => now true (even in Ruby 1.9)

There is also a problem with DateTime vs TimeWithZone:

dt = foo.to_datetime
foo.eql?(dt)  # => true
dt.eql?(foo)  # => false
dt.hash == foo # => false.

Since DateTime and a Time are both builtin classes and that a Time is never eql? to a DateTime or vice-versa (as per the doc and the implementation), the patch also insures that a TimeWithZone can not be eql? to a DateTime:

foo.eql?(dt)  # => now false

Thanks

Imported from Lighthouse.
Comment by Pan Thomakos - 2011-03-12 01:26:38 UTC

It seems to me that these objects should behave like regular ruby objects do. For instance:

10 == 10.00 # Same value
=> true
10.eql?(10.00) # Same value different type
=> false

Using the latter code as an example, I would expect Time and TimeWithZone to behave like this:

bar = Time.utc(2000)
foo = bar.in_time_zone
foo.eql?(bar)
=> false
bar.eql?(foo)
=> false

It seems like making eql? more lenient is removing that flexibility in testing for types. I do believe that foo.eql?(bar) should produce the same result as bar.eql?(foo), but I think that if foo and bar are of different types (Time) and (ActiveSupport::TimeWithZone) the eql? comparison should resolve to false.

Imported from Lighthouse.
Comment by Marc-André Lafortune - 2011-03-12 18:28:58 UTC

Floats and Integers are quite different: there is no bijection between them and they don't respond to the same methods (e.g. you can't do 10.0.times{}).

Note that regular objects do not distinguish class with eql?:

class MyString < String; end
MyString.new("foo").eql?("foo") # => true

In any case, one shouldn't care about class/type but about what objects respond to.

Unless I'm mistaken, TimeWithZone is the same as Time, except that it allows for a timezone information and that timezone information is not considered when doing comparisons.

TimeWithZone is meant to be so similar to a Time that TimeWithZone#to_time returns... a TimeWithZone. My impression is that one shouldn't have to care too much about what is a Time and what is a TimeWithZone; I'd like to use Hash lookups without caring either.

Thanks.

Imported from Lighthouse.
Comment by Pan Thomakos - 2011-03-12 20:29:31 UTC

What you write makes sense. After taking a look at the class documentation this does seem to be the intent.

I like your implementation, it's simple and elegant, and I was able to get tests to pass in both 1.9.2 and 1.8.7.

You mentioned that your patch ensures that TimeWithZone cannot eql? a DateTime. Unless I am mistaken I didn't see a test for this. Would it be worth adding one?

Imported from Lighthouse.
Comment by Marc-André Lafortune - 2011-03-13 19:48:33 UTC

Thanks for testing on your side.

Yes, it could be worthwhile to add a test as you suggest.

Imported from Lighthouse.
Comment by Ryan Orr - 2011-03-21 02:49:12 UTC

I too ran the code that you made and thought it made sense. I added the test that Pan Thomakos discussed in this updated patch.

Attachments saved to Gist: http://gist.github.com/971805

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