Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix inconsistencies with Time{WithZone}#{hash,eql?} #3725

Merged
merged 1 commit into from Nov 22, 2011
Merged

Fix inconsistencies with Time{WithZone}#{hash,eql?} #3725

merged 1 commit into from Nov 22, 2011

Conversation

marcandre
Copy link
Contributor

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

This pull request 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 no longer be eql? to a DateTime:

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

Thanks

Extended discussion at #905

wycats added a commit that referenced this pull request Nov 22, 2011
Fix inconsistencies with Time{WithZone}#{hash,eql?}
@wycats wycats merged commit a93ee92 into rails:master Nov 22, 2011
repinel added a commit to repinel/rails that referenced this pull request Jul 19, 2015
Before:

```ruby
  twz = DateTime.now.in_time_zone
  twz.eql?(twz.dup) => false
```

Now:

```ruby
  twz = DateTime.now.in_time_zone
  twz.eql?(twz.dup) => true
```

Please notice that this fix the `TimeWithZone` comparison to itself,
not to `DateTime`. Based on rails#3725, `DateTime` should not be equal to
`TimeWithZone`.
repinel added a commit to repinel/rails that referenced this pull request Oct 5, 2015
Before:

```ruby
  twz = DateTime.now.in_time_zone
  twz.eql?(twz.dup) => false
```

Now:

```ruby
  twz = DateTime.now.in_time_zone
  twz.eql?(twz.dup) => true
```

Please notice that this fix the `TimeWithZone` comparison to itself,
not to `DateTime`. Based on rails#3725, `DateTime` should not be equal to
`TimeWithZone`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants