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

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.

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.

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?

Thanks for testing on your side.

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

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.