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 ruby Enumerable#sum if available #25202

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
9 participants
@vipulnsward
Member

vipulnsward commented May 30, 2016

Enumerable#sum was added in ruby/ruby@41ef7ec

  • use Enumerable#sum if its available, to replicate how we enhance on usage of native Aray sum.
  • rm extra check that passed identity is numeric for Array#sum

r? @jeremy

Show outdated Hide outdated activesupport/lib/active_support/core_ext/enumerable.rb
@@ -119,7 +134,7 @@ class Array
alias :orig_sum :sum
def sum(init = nil, &block) #:nodoc:
if init.is_a?(Numeric) || first.is_a?(Numeric)
if init || first.is_a?(Numeric)

This comment has been minimized.

@jeremy

jeremy May 30, 2016

Member

Why? Removing the Numeric check because other objects are supported now?

@jeremy

jeremy May 30, 2016

Member

Why? Removing the Numeric check because other objects are supported now?

This comment has been minimized.

@vipulnsward

vipulnsward May 30, 2016

Member

This comment has been minimized.

@vipulnsward

vipulnsward May 30, 2016

Member

Actually my mistake, its not interchangeable for all objects. probably Enumerable isn't too, let me see.

@vipulnsward

vipulnsward May 30, 2016

Member

Actually my mistake, its not interchangeable for all objects. probably Enumerable isn't too, let me see.

This comment has been minimized.

@jeremy

jeremy May 30, 2016

Member

Ah right, supports other objects if an init arg is provided. It'd be clearer to split up the conditional, then:

if init
  orig_sum(init, &block)
elsif first.is_a?(Numeric)
  orig_sum(0, &block)
else
  super
end

Since Ruby supports other objects now, we could go even further:

if init
  orig_sum(init, &block)
elsif init = _identity_for_sum
  orig_sum(init, &block)
else
  super
end

private def _identity_for_sum #:nodoc:
  case first
  when Numeric; 0
  when String; ''
  when Array; []
  …
  end
end
@jeremy

jeremy May 30, 2016

Member

Ah right, supports other objects if an init arg is provided. It'd be clearer to split up the conditional, then:

if init
  orig_sum(init, &block)
elsif first.is_a?(Numeric)
  orig_sum(0, &block)
else
  super
end

Since Ruby supports other objects now, we could go even further:

if init
  orig_sum(init, &block)
elsif init = _identity_for_sum
  orig_sum(init, &block)
else
  super
end

private def _identity_for_sum #:nodoc:
  case first
  when Numeric; 0
  when String; ''
  when Array; []
  …
  end
end

This comment has been minimized.

@jeremy

jeremy May 30, 2016

Member

We could also take the opportunity to remove AS #sum entirely and rely on Ruby 2.4+ behavior for the future. It's so close to AS sum now; the only difference is requiring the initial value.

We could deprecate calling #sum on a non-Numeric Enumerable without an initial arg, then break compat in 5.1, and remove our #sum entirely in Rails 6.

@jeremy

jeremy May 30, 2016

Member

We could also take the opportunity to remove AS #sum entirely and rely on Ruby 2.4+ behavior for the future. It's so close to AS sum now; the only difference is requiring the initial value.

We could deprecate calling #sum on a non-Numeric Enumerable without an initial arg, then break compat in 5.1, and remove our #sum entirely in Rails 6.

Show outdated Hide outdated activesupport/lib/active_support/core_ext/enumerable.rb
else
_enum_sum(identity, &block)
end
end

This comment has been minimized.

@jeremy

jeremy May 30, 2016

Member

extra indentation

@jeremy

jeremy May 30, 2016

Member

extra indentation

@jeremy jeremy added this to the 5.0.1 milestone May 30, 2016

Show outdated Hide outdated activesupport/lib/active_support/core_ext/enumerable.rb
@@ -17,7 +17,7 @@ module Enumerable
# The default sum of an empty list is zero. You can override this default:
#
# [].sum(Payment.new(0)) { |i| i.amount } # => Payment.new(0)
def sum(identity = nil, &block)
def _enum_sum(identity = nil, &block)

This comment has been minimized.

@yui-knk

yui-knk May 31, 2016

Contributor

I think it is not good to define public method even if method name starting with _.
And I feel inlining this method is better than a private method.

@yui-knk

yui-knk May 31, 2016

Contributor

I think it is not good to define public method even if method name starting with _.
And I feel inlining this method is better than a private method.

else
sum = identity ? inject(identity, :+) : inject(:+)
sum || identity || 0
end

This comment has been minimized.

@vipulnsward

vipulnsward May 31, 2016

Member

hmm, the inlining makes this pretty much grow a lot.

@vipulnsward

vipulnsward May 31, 2016

Member

hmm, the inlining makes this pretty much grow a lot.

This comment has been minimized.

@jeremy

jeremy May 31, 2016

Member

Probably worth it in this case :)

@jeremy

jeremy May 31, 2016

Member

Probably worth it in this case :)

Show outdated Hide outdated activesupport/lib/active_support/core_ext/enumerable.rb
return 0 if first.is_a?(Numeric)
ActiveSupport::Deprecation.warn(<<-MSG.squish)
Support for calling #sum on a non-numeric enumerable without an identity argument,
is deprecated and will soon be removed.

This comment has been minimized.

@vipulnsward

vipulnsward May 31, 2016

Member

@jeremy something like this?

@vipulnsward

vipulnsward May 31, 2016

Member

@jeremy something like this?

This comment has been minimized.

@jeremy

jeremy May 31, 2016

Member

Yeah, though the warning belongs in #sum itself. Since we don't want to warn if the elements are numeric, we could split that conditional out:

if identity
  …
elsif first.is_a?(Numeric)
  …
elsif identity = _identity_for_sumelsif block_given?elseend
@jeremy

jeremy May 31, 2016

Member

Yeah, though the warning belongs in #sum itself. Since we don't want to warn if the elements are numeric, we could split that conditional out:

if identity
  …
elsif first.is_a?(Numeric)
  …
elsif identity = _identity_for_sumelsif block_given?elseend

This comment has been minimized.

@vipulnsward

vipulnsward May 31, 2016

Member

Yeah, that is how this is being handled right now, see the return in case of Numeric.

@vipulnsward

vipulnsward May 31, 2016

Member

Yeah, that is how this is being handled right now, see the return in case of Numeric.

@@ -1,3 +1,4 @@
require 'active_support/deprecation'

This comment has been minimized.

@jeremy

jeremy May 31, 2016

Member

Newline between require and module declaration

@jeremy

jeremy May 31, 2016

Member

Newline between require and module declaration

Show outdated Hide outdated activesupport/lib/active_support/core_ext/enumerable.rb
sum || identity || 0
end
end
if Enumerable.instance_methods(false).include?(:sum) && !(%w[a].sum rescue false)
alias :orig_enum_sum :sum

This comment has been minimized.

@jeremy

jeremy May 31, 2016

Member

Still have four spaces indentation here instead of two :)

@jeremy

jeremy May 31, 2016

Member

Still have four spaces indentation here instead of two :)

Show outdated Hide outdated activesupport/lib/active_support/core_ext/enumerable.rb
super
end
def sum(identity = nil, &block) #:nodoc:
super

This comment has been minimized.

@jeremy

jeremy May 31, 2016

Member

Ruby has separate Array#sum and Enumerable#sum implementations, so we'll want to continue using orig_sum for the optimized Array implementation but calling super to get our compatible #sum behavior.

@jeremy

jeremy May 31, 2016

Member

Ruby has separate Array#sum and Enumerable#sum implementations, so we'll want to continue using orig_sum for the optimized Array implementation but calling super to get our compatible #sum behavior.

@vipulnsward

This comment has been minimized.

Show comment
Hide comment
@vipulnsward

vipulnsward May 31, 2016

Member

Ok, update. How does this look.

Member

vipulnsward commented May 31, 2016

Ok, update. How does this look.

vipulnsward added some commits May 30, 2016

Enumerable#sum was added in ruby/ruby@41ef7ec
- use Enumerable#sum if its available.
- rm extra check that passed identity is numeric
@vipulnsward

This comment has been minimized.

Show comment
Hide comment
@vipulnsward

vipulnsward Jul 9, 2016

Member

Did another rebase, test failure is unrelated.

Member

vipulnsward commented Jul 9, 2016

Did another rebase, test failure is unrelated.

else
sum = identity ? inject(identity, :+) : inject(:+)
sum || identity || 0
if Enumerable.instance_methods(false).include?(:sum) && !(%w[a].sum rescue false)

This comment has been minimized.

@rwz

rwz Nov 3, 2016

Contributor

what about method_defined?(:sum). Seems more straightforward and concise

@rwz

rwz Nov 3, 2016

Contributor

what about method_defined?(:sum). Seems more straightforward and concise

return 0 if first.is_a?(Numeric)
ActiveSupport::Deprecation.warn(<<-MSG.squish)
Support for calling #sum on a non-numeric enumerable without an identity argument,
is deprecated and will soon be removed.

This comment has been minimized.

@matthewd

matthewd Nov 3, 2016

Member

Do we actually agree with this? It's nice that upstream has partially adopted the API and optimized it, but we shouldn't lose sight of the fact we added it to make code look better... it's possible we still prefer our original, more generous, API. I certainly do.

OTOH, if we have in fact changed our mind about the API, this deprecation should apply on earlier Ruby versions too.

@matthewd

matthewd Nov 3, 2016

Member

Do we actually agree with this? It's nice that upstream has partially adopted the API and optimized it, but we shouldn't lose sight of the fact we added it to make code look better... it's possible we still prefer our original, more generous, API. I certainly do.

OTOH, if we have in fact changed our mind about the API, this deprecation should apply on earlier Ruby versions too.

end
else
def sum(identity = nil, &block)
if block_given?

This comment has been minimized.

@jmolina91

jmolina91 Nov 4, 2016

You are repeating this code twice, maybe you can abstract it in a private common method

@jmolina91

jmolina91 Nov 4, 2016

You are repeating this code twice, maybe you can abstract it in a private common method

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Dec 31, 2016

Member

@vipulnsward you still interested in picking this back up again? 😊

Member

kaspth commented Dec 31, 2016

@vipulnsward you still interested in picking this back up again? 😊

@kamipo

This comment has been minimized.

Show comment
Hide comment
@kamipo

kamipo Apr 19, 2017

Member

Closing in favor of #28781.

Member

kamipo commented Apr 19, 2017

Closing in favor of #28781.

@kamipo kamipo closed this Apr 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment