Permalink
Browse files

Merge branch 'deprecate-calculations-with-block'

Follow up of the discussion from the original merge commit:
f9cb645#commitcomment-1414561

We want to avoid people's mistakes with methods like count and sum when
called with a block, that can easily lead to code performing poorly and
that could be way better written with a db query.

Please check the discussion there for more background.
Closes #8268
  • Loading branch information...
2 parents ae934ae + fd1c457 commit a9aeba671d7af941b790afa7d0126f5b7c52aa7f @carlosantoniodasilva carlosantoniodasilva committed Nov 22, 2012
View
@@ -1,5 +1,10 @@
## Rails 4.0.0 (unreleased) ##
+* Deprecate calling `Relation#sum` with a block. To perform a calculation over
+ the array result of the relation, use `to_a.sum(&block)`.
+
+ *Carlos Antonio da Silva*
+
* Fix postgresql adapter to handle BC timestamps correctly
HistoryEvent.create!(:name => "something", :occured_at => Date.new(0) - 5.years)
@@ -736,13 +741,6 @@
*Marc-André Lafortune*
-* Allow blocks for `count` with `ActiveRecord::Relation`, to work similar as
- `Array#count`:
-
- Person.where("age > 26").count { |person| person.gender == 'female' }
-
- *Chris Finne & Carlos Antonio da Silva*
-
* Added support to `CollectionAssociation#delete` for passing `fixnum`
or `string` values as record ids. This finds the records responding
to the `id` and executes delete on them.
@@ -234,7 +234,7 @@ def association_instance_set(name, association)
# others.size | X | X | X
# others.length | X | X | X
# others.count | X | X | X
- # others.sum(args*,&block) | X | X | X
+ # others.sum(*args) | X | X | X
# others.empty? | X | X | X
# others.clear | X | X | X
# others.delete(other,other,...) | X | X | X
@@ -161,15 +161,6 @@ def destroy_all
end
end
- # Calculate sum using SQL, not Enumerable.
- def sum(*args)
- if block_given?
- scope.sum(*args) { |*block_args| yield(*block_args) }
- else
- scope.sum(*args)
- end
- end
-
# Count all records using SQL. If the +:counter_sql+ or +:finder_sql+ option is set for the
# association, it will be used for the query. Otherwise, construct options and pass them with
# scope to the target class's +count+.
@@ -1,3 +1,5 @@
+require 'active_support/deprecation'
+
module ActiveRecord
module Calculations
# Count the records.
@@ -13,16 +15,9 @@ module Calculations
#
# Person.count(:age, distinct: true)
# # => counts the number of different age values
- #
- # Person.where("age > 26").count { |person| person.gender == 'female' }
- # # => queries people where "age > 26" then count the loaded results filtering by gender
def count(column_name = nil, options = {})
- if block_given?
- self.to_a.count { |item| yield item }
- else
- column_name, options = nil, column_name if column_name.is_a?(Hash)
- calculate(:count, column_name, options)
- end
+ column_name, options = nil, column_name if column_name.is_a?(Hash)
+ calculate(:count, column_name, options)
end
# Calculates the average value on a given column. Returns +nil+ if there's
@@ -56,13 +51,13 @@ def maximum(column_name, options = {})
# +calculate+ for examples with options.
#
# Person.sum('age') # => 4562
- # # => returns the total sum of all people's age
- #
- # Person.where('age > 100').sum { |person| person.age - 100 }
- # # queries people where "age > 100" then perform a sum calculation with the block returns
def sum(*args)
if block_given?
- self.to_a.sum(*args) { |item| yield item }
+ ActiveSupport::Deprecation.warn(
+ "Calling #sum with a block is deprecated and will be removed in Rails 4.1. " \
+ "If you want to perform sum calculation over the array of elements, use `to_a.sum(&block)`."
+ )
+ self.to_a.sum(*args) {|*block_args| yield(*block_args)}
else
calculate(:sum, *args)
end
@@ -1658,6 +1658,12 @@ def test_collection_association_with_private_kernel_method
assert_deprecated { klass.has_many :foo, :counter_sql => 'lol' }
end
+ test "sum calculation with block for array compatibility is deprecated" do
+ assert_deprecated do
+ posts(:welcome).comments.sum { |c| c.id }
+ end
+ end
+
test "has many associations on new records use null relations" do
post = Post.new
@@ -383,30 +383,16 @@ def test_count_with_from_option
Company.where(:type => "Firm").from('companies').count(:type)
end
- def test_count_with_block_acts_as_array
- accounts = Account.where('id > 0')
- assert_equal Account.count, accounts.count { true }
- assert_equal 0, accounts.count { false }
- assert_equal Account.where('credit_limit > 50').size, accounts.count { |account| account.credit_limit > 50 }
- assert_equal Account.count, Account.count { true }
- assert_equal 0, Account.count { false }
- end
-
- def test_sum_with_block_acts_as_array
- accounts = Account.where('id > 0')
- assert_equal Account.sum(:credit_limit), accounts.sum { |account| account.credit_limit }
- assert_equal Account.sum(:credit_limit) + Account.count, accounts.sum{ |account| account.credit_limit + 1 }
- assert_equal 0, accounts.sum { |account| 0 }
- end
-
def test_sum_with_from_option
assert_equal Account.sum(:credit_limit), Account.from('accounts').sum(:credit_limit)
assert_equal Account.where("credit_limit > 50").sum(:credit_limit),
Account.where("credit_limit > 50").from('accounts').sum(:credit_limit)
end
- def test_sum_array_compatibility
- assert_equal Account.sum(:credit_limit), Account.sum(&:credit_limit)
+ def test_sum_array_compatibility_deprecation
+ assert_deprecated do
+ assert_equal Account.sum(:credit_limit), Account.sum(&:credit_limit)
+ end
end
def test_average_with_from_option

0 comments on commit a9aeba6

Please sign in to comment.