Support for unified Integer class in Ruby 2.4+ #25056

Merged
merged 1 commit into from May 19, 2016

Projects

None yet

6 participants

@jeremy
Member
jeremy commented May 17, 2016

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

  • Forward compat with new unified Integer class in Ruby 2.4+.
  • Backward compat with separate Fixnum/Bignum in Ruby 2.2 & 2.3.
  • Drops needless Fixnum distinction in docs, preferring Integer.
@kaspth
Member
kaspth commented May 17, 2016

LGTM :shipit:

@sgrif sgrif commented on the diff May 18, 2016
...rt/lib/active_support/core_ext/numeric/conversions.rb
@@ -134,6 +134,12 @@ def to_formatted_s(*args)
deprecate to_formatted_s: :to_s
end
-[Fixnum, Bignum, Float, BigDecimal].each do |klass|
- klass.prepend(ActiveSupport::NumericWithFormat)
+# Ruby 2.4+ unifies Fixnum & Bignum into Integer.
+if Integer == Fixnum
@sgrif
sgrif May 18, 2016 Member

Didn't the feature mention warning when Fixnumis directly referenced? Should we check for the ruby version explicitly instead?

@jeremy
jeremy May 18, 2016 Member

It's undecided; there's no warning currently. It may be too noisy to tolerate for widespread use. Prefer feature detection to implementation version checks.

@sgrif
sgrif May 18, 2016 Member

Fair enough. If nothing else we could move the Integer.prepend case outside of the conditional. Perhaps something like this:

Integer.prepend(ActiveSupport::NumericWithFormat)

if defined?(Bignum) && Bignum.ancestors.exclude?(ActiveSupport::NumericWithFormat)
  Bignum.prepend(ActiveSupport::NumericWithFormat)
end
@jeremy
jeremy May 18, 2016 Member

Thanks for the suggestions. That prepends Fixnum's ancestor, not Fixnum, so no go.

To avoid referencing Fixnum entirely, the simple route is checking 1.class == Integer. I didn't litter the code with this (there are half a dozen other cases) because it's speculating how—and whether—Fixnum references will spam warnings. Poor trade for unknown benefit. There's little drawback to waiting to see what drops on Ruby trunk.

@sgrif
sgrif May 18, 2016 Member

👍

On Wed, May 18, 2016, 6:30 PM Jeremy Daer notifications@github.com wrote:

In activesupport/lib/active_support/core_ext/numeric/conversions.rb
#25056 (comment):

@@ -134,6 +134,12 @@ def to_formatted_s(*args)
deprecate to_formatted_s: :to_s
end

-[Fixnum, Bignum, Float, BigDecimal].each do |klass|

  • klass.prepend(ActiveSupport::NumericWithFormat)
    +# Ruby 2.4+ unifies Fixnum & Bignum into Integer.
    +if Integer == Fixnum

Thanks for the suggestions. That prepends Fixnum's ancestor, not Fixnum,
so no go.

To avoid referencing Fixnum entirely, the simple route is checking 1.class
== Integer. I didn't litter the code with this (there are half a dozen
other cases) because it's speculating how—and whether—Fixnum references
will spam warnings. Poor trade for unknown benefit. There's little drawback
to waiting to see what drops on Ruby trunk.


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
https://github.com/rails/rails/pull/25056/files/01d1a9f97fdda379ed47a737d2416b2139209cb7#r63794236

@jeremy jeremy Support for unified Integer class in Ruby 2.4+
Ruby 2.4 unifies Fixnum and Bignum into Integer: https://bugs.ruby-lang.org/issues/12005

* Forward compat with new unified Integer class in Ruby 2.4+.
* Backward compat with separate Fixnum/Bignum in Ruby 2.2 & 2.3.
* Drops needless Fixnum distinction in docs, preferring Integer.
89e2f7e
@jeremy jeremy merged commit 89e2f7e into rails:master May 19, 2016

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
@jeremy jeremy deleted the jeremy:ruby24-integer-unification branch May 19, 2016
@jeremy jeremy added this to the 5.0.1 milestone May 19, 2016
@jeremy jeremy added the backport label May 19, 2016
@jeremy
Member
jeremy commented May 19, 2016

5-0-stable: dd7d5b7

@vipulnsward
Member

@jeremy needs a CHANGELOG?

@jeremy
Member
jeremy commented May 19, 2016

@vipulnsward I wrote one but removed it. From users' point of view, nothing changes.

@vipulnsward
Member

Probably the class name? Not sure if people might rely on it though, so 👍

@rafaelfranca
Member

👍

@maclover7 maclover7 removed the backport label Jun 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment