Fix ActiveSupport::Duration#instance_of? to behave like #is_a?
#16662
Conversation
| assert !1.minute.instance_of?(Fixnum) | ||
| assert !2.days.instance_of?(Fixnum) | ||
| assert 1.minute.instance_of?(Fixnum) | ||
| assert 2.days.instance_of?(ActiveSupport::Duration) |
There was a problem hiding this comment.
Can you move this closer to test_is_a? Also, should this includes some of the test cases in there?
There was a problem hiding this comment.
The test has been moved under test_is_a. Sorry actually I mess between value.is_a?(klass) and value.instance_of?(klass). ActiveSupport::Duration's #instance_of? should not behave like #is_a? ; it should just return true for itself and Fixnum.
Thanks for your review ; good call ! :-)
For the sake of backward-compatibility, we need to make #instance_of? return true for Fixnum. On the other hand, the method should still give true for ActiveSupport::Duration itself which was not the case before.
Since Rubinius is relying on #instance_of? for its definition of #eql? (http://git.io/MtmbbA) but ActiveSupport::Duration should behave like is_a? it returns true with `Fixnum`. Thus, for the moment, the last assertion is failing so we have to skip this test.
Fix `ActiveSupport::Duration#instance_of?` to behave like `#is_a?`
| @@ -35,17 +41,15 @@ def test_equals | |||
| end | |||
|
|
|||
| def test_eql | |||
| rubinius_skip "Rubinius' #eql? definition relies on #instance_of? " \ | |||
| "which behaves oddly for the sake of backward-compatibility." | |||
There was a problem hiding this comment.
@robin850 should this be reported as a bug to rubinius?
There was a problem hiding this comment.
I'm not sure that they can do something on their end but I may be wrong. Numeric#eql? will call #instance_of? on the given object. The problem is that since Rubinius is written in Ruby, when we are dealing with core objects, we should try to follow as much as possible the vanilla behavior ; this is certainly another case of Rubinius wants to help you make Ruby better.
In my opinion, this is something to fix on our end for 5.0. At the moment, #instance_of? doesn't behave in an idiomatic way ; we may have to completely refactor this layer (unless we simply don't want to be BC with this method in 5.0).
Hello,
This pull request is a follow-up to #16560. The latter introduced a regression as
ActiveSupport::Duration#instance_of?should return true withFixnumto make this layer "transparent" as people most likely consider1.dayas a fixnum. Thus this pull request makes#instance_of?behave like#is_a?as this was previously returning false for the class itself since it was delegated to thevalueattribute.A second commit has also been attached to skip the
#eql?tests on Rubinius since the first purpose of #16560 was to make those tests to pass.Have a nice day.