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

Use Module.prepend instead of alias_method and unify behavior of all Numeric extensions #20038

Merged
merged 1 commit into from Oct 20, 2015

Conversation

imanel
Copy link
Contributor

@imanel imanel commented May 5, 2015

This is a follow-up pull request to #19434 - now that Ruby 2.2.2 is out we can switch to #prepend.

/cc @rafaelfranca @kirs

@imanel
Copy link
Contributor Author

imanel commented May 5, 2015

Build is passing - not sure why travis is reporting it's still building.

@kirs
Copy link
Member

kirs commented May 6, 2015

Looks good 👍
Hope we could use refinements at some point.

On Wednesday, May 6, 2015, Bernard Potocki notifications@github.com wrote:

Build is passing - not sure why travis is reporting it's still building.


Reply to this email directly or view it on GitHub
#20038 (comment).

@imanel
Copy link
Contributor Author

imanel commented May 12, 2015

@rafaelfranca ?

@rafaelfranca
Copy link
Member

We can't change this too easily because there aliases are public API. We could deprecate them but I'm not sure if we will have any improvement on doing so.

@rafaelfranca
Copy link
Member

Oops, I should not have closed.

@carlosantoniodasilva @senny @chancancode WDYT?

@carlosantoniodasilva
Copy link
Member

I think those methods only existed due to the alias method chain stuff, so I'd be game to deprecating them now that we can use prepend. Extremely low priority thing to do, though.

On another topic, it freaks me out moving code like that because of indent =(, I wonder if declaring the modules as module ActiveSupport::NumericWithFormat wouldn't make it easier.

@imanel
Copy link
Contributor Author

imanel commented May 28, 2015

@carlosantoniodasilva removed indentation, added deprecated to_formatted_s. I've used def instead of alias so we can still overwrite it in BigDecimal.


[Fixnum, Bignum, Float, BigDecimal].each do |klass|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't this just be Numeric.prepend?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be delighted to do so, but we've never overwritten Complex and Rational so I hesitated a little:

>> ObjectSpace.each_object(Class).select { |klass| klass < Numeric }
#=> [Complex, Rational, Bignum, Float, Fixnum, Integer]

and it looks like it will not work properly:

>> ActiveSupport::NumberHelper.number_to_percentage Complex(2,2)
#=> RangeError: can't convert 2+2i into Float

sgrif added a commit that referenced this pull request Oct 20, 2015
Use Module.prepend instead of alias_method and unify behavior of all Numeric extensions
@sgrif sgrif merged commit 7e434d6 into rails:master Oct 20, 2015
@sgrif
Copy link
Contributor

sgrif commented Oct 20, 2015

This seems like a fine improvement from the implementation side. If we're ever going to deprecate the aliases which were really only there for implementation reasons, Rails 5 is the version to do it.

sgrif added a commit that referenced this pull request Oct 20, 2015
We use one period after spaces, not two.
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

6 participants