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

Refining Array#sum monkey-patch using Refinements #27363

Merged
merged 1 commit into from Jan 3, 2017

Conversation

Projects
None yet
8 participants
@amatsuda
Member

amatsuda commented Dec 14, 2016

Here's an alternative implementation of Array#sum.

Current Array#sum unwantedly exposes our own internal orig_sum method to the outside world.
Ruby 2's probably most unused but useful new feature "Refinements" is provided exactly for this use case to define a perfectly private method that can be accessed only inside this file.

I'd like to ask the core members' opinion about this approach, since AFAIK we've never used Refinements in our code base before.
If you like it, I'd love to rewrite some other monkey-patches to this style.
And I guess #25202 can also be more elegantly solved using this technique.

/cc @jeremy

Refining Array#sum monkey-patch using Refinements
Because we don't want to see our ugly orig_sum method outside of this file
@yui-knk

This comment has been minimized.

Show comment
Hide comment
@yui-knk

yui-knk Dec 15, 2016

Contributor

Looks good to me 👍
I wrote Use original Array#sum to speed up calculating.
A defect of alias :orig_sum :sum is to expose #orig_sum to Rails users.
This commit will solve the problem because only Array#sum can access #orig_sum.

I think Refinements are best tool to solve this problem :)

Contributor

yui-knk commented Dec 15, 2016

Looks good to me 👍
I wrote Use original Array#sum to speed up calculating.
A defect of alias :orig_sum :sum is to expose #orig_sum to Rails users.
This commit will solve the problem because only Array#sum can access #orig_sum.

I think Refinements are best tool to solve this problem :)

@kamipo

This comment has been minimized.

Show comment
Hide comment
@kamipo

kamipo Dec 19, 2016

Member

How about private :orig_sum?

Member

kamipo commented Dec 19, 2016

How about private :orig_sum?

@yui-knk

This comment has been minimized.

Show comment
Hide comment
@yui-knk

yui-knk Dec 19, 2016

Contributor

We can call a private method by send :orig_sum, so I prefer to Refinements in this context :)

Contributor

yui-knk commented Dec 19, 2016

We can call a private method by send :orig_sum, so I prefer to Refinements in this context :)

@kamipo

This comment has been minimized.

Show comment
Hide comment
@kamipo

kamipo Dec 19, 2016

Member

I see :)

Member

kamipo commented Dec 19, 2016

I see :)

@sgrif sgrif merged commit a194ec8 into rails:master Jan 3, 2017

2 checks passed

codeclimate no new or fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jan 3, 2017

Member

I'm fine with using refinements but can we change our monkey-patches to this styles even that refinements are only in the file context?

Member

rafaelfranca commented Jan 3, 2017

I'm fine with using refinements but can we change our monkey-patches to this styles even that refinements are only in the file context?

@amatsuda amatsuda deleted the amatsuda:refined_array_sum branch Jan 4, 2017

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Jan 6, 2017

Contributor

a late comment ... why does this need to be hidden that much esp. in an open language such as Ruby?
I understand that it seems like a good fit here but have you considered the impact on alternate rubies?

maybe there's a reason why its the most unused feature (besides doing terrible on JRuby) - just guessing
... in general when a Ruby method (esp. from a library) wasn't working as expected ✂️ worked well:
-> its probably not that easy to do that with refinements? would you guys consider an alternative here?

Contributor

kares commented Jan 6, 2017

a late comment ... why does this need to be hidden that much esp. in an open language such as Ruby?
I understand that it seems like a good fit here but have you considered the impact on alternate rubies?

maybe there's a reason why its the most unused feature (besides doing terrible on JRuby) - just guessing
... in general when a Ruby method (esp. from a library) wasn't working as expected ✂️ worked well:
-> its probably not that easy to do that with refinements? would you guys consider an alternative here?

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jan 6, 2017

Member

I understand that it seems like a good fit here but have you considered the impact on alternate rubies?

Officially we only support MRI, JRuby support is still work in progress. But which impacts have refinements on alternate rubies?

Member

rafaelfranca commented Jan 6, 2017

I understand that it seems like a good fit here but have you considered the impact on alternate rubies?

Officially we only support MRI, JRuby support is still work in progress. But which impacts have refinements on alternate rubies?

@printercu

This comment has been minimized.

Show comment
Hide comment
@printercu

printercu Jan 19, 2017

Contributor

UPD Sorry, misunderstood the original purpose of PR.

@amatsuda @yui-knk Maybe it'll be better to use Array.prepend with new new #sum implementation? Something like this:

  module ActiveSupport::NumericSum
     def sum(init = nil, *, &block) #:nodoc:
       init ||= 0 if !init && first.is_a?(Numeric)
       super
     end
  end
  Array.prepend(ActiveSupport::NumericSum)
Contributor

printercu commented Jan 19, 2017

UPD Sorry, misunderstood the original purpose of PR.

@amatsuda @yui-knk Maybe it'll be better to use Array.prepend with new new #sum implementation? Something like this:

  module ActiveSupport::NumericSum
     def sum(init = nil, *, &block) #:nodoc:
       init ||= 0 if !init && first.is_a?(Numeric)
       super
     end
  end
  Array.prepend(ActiveSupport::NumericSum)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment