Skip to content

Conversation

@yui-knk
Copy link

@yui-knk yui-knk commented May 29, 2016

Ruby 2.4 unifies Fixnum and Bignum into Integer: https://bugs.ruby-lang.org/issues/12005

Ruby ~2.3 1234.class is Fixnum and 123456789012345678901234567890.class
is Bignum.
Ruby 2.4+ 1234.class is Integer and 123456789012345678901234567890.class
is Integer.

So what we should do is defining visit_Integer method to visitors.

Ruby 2.4 unifies Fixnum and Bignum into Integer: https://bugs.ruby-lang.org/issues/12005

Ruby ~2.3 `1234.class` is `Fixnum` and `123456789012345678901234567890.class`
is `Bignum`.
Ruby 2.4+ `1234.class` is `Integer` and `123456789012345678901234567890.class`
is `Integer`.

So what we should do is defining `visit_Integer` method to visitors.
@rails-bot
Copy link

r? @sgrif

(@rails-bot has picked a reviewer for you, use r? to override)

@vipulnsward
Copy link
Member

cc @jeremy

@vipulnsward
Copy link
Member

Do we also want to restrict support for visit_Bignum and visit_Fixnum for < 2.4 ?

@jeremy jeremy merged commit dc85a6e into rails:master May 29, 2016
jeremy added a commit that referenced this pull request May 29, 2016
Support for unified Integer class in Ruby 2.4+
@sgrif
Copy link

sgrif commented May 29, 2016

No, I think it's fine to leave them as-is

@yui-knk yui-knk deleted the fix_head branch May 30, 2016 07:01
prathamesh-sonpatki added a commit to prathamesh-sonpatki/rails that referenced this pull request Jul 24, 2016
… unification

* Following tests were failing on Ruby edge 2.4 version -

- RelationTest#test_update_all_with_joins_and_offset_and_order:
- RelationTest#test_update_all_with_joins_and_offset:
- BasicsTest#test_no_limit_offset:
- CalculationsTest#test_offset_is_kept:
- ActiveRecord::CollectionCacheKeyTest#test_cache_key_for_queries_with_offset_which_return_0_rows:
- FinderTest#test_third_to_last:

* As Arel 7.1 supports Integer unification after rails/arel#437 we can use it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

5 participants