This project is archived and is in readonly mode.

#6534 new
Marc-André Lafortune

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

Reported by Marc-André Lafortune | March 5th, 2011 @ 08:51 PM

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

Comments and changes to this ticket

  • Marc-André Lafortune

    Marc-André Lafortune March 5th, 2011 @ 08:52 PM

    • no changes were found...
  • Pan Thomakos

    Pan Thomakos March 12th, 2011 @ 01:26 AM

    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.

  • Marc-André Lafortune

    Marc-André Lafortune March 12th, 2011 @ 06:28 PM

    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.

  • Pan Thomakos

    Pan Thomakos March 12th, 2011 @ 08:29 PM

    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?

  • Marc-André Lafortune

    Marc-André Lafortune March 13th, 2011 @ 07:48 PM

    Thanks for testing on your side.

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

  • Ryan Orr

    Ryan Orr March 21st, 2011 @ 02:49 AM

    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.

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

People watching this ticket

Attachments

Tags

Pages