From 76a6bfd6c8dc9e3c27779a39b3e04402e20f0eaa Mon Sep 17 00:00:00 2001 From: Carlos Antonio da Silva Date: Sat, 17 Nov 2012 18:57:32 -0200 Subject: [PATCH 1/3] Revert "Yield only one argument instead of splatting." This reverts commit f9cb645dfcb5cc89f59d2f8b58a019486c828c73. Conflicts: activerecord/CHANGELOG.md Revert "Allow blocks for count with ActiveRecord::Relation. Document and test that sum allows blocks" This reverts commit 9cc2bf69ce296b7351dc612a8366193390a305f3. Conflicts: activerecord/lib/active_record/relation/calculations.rb --- activerecord/CHANGELOG.md | 7 ------- .../lib/active_record/relation/calculations.rb | 17 +++-------------- activerecord/test/cases/calculations_test.rb | 16 ---------------- 3 files changed, 3 insertions(+), 37 deletions(-) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index f166d738014c7..22e57a6774f9d 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -736,13 +736,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. diff --git a/activerecord/lib/active_record/relation/calculations.rb b/activerecord/lib/active_record/relation/calculations.rb index 741f94f777573..24f3c128d102d 100644 --- a/activerecord/lib/active_record/relation/calculations.rb +++ b/activerecord/lib/active_record/relation/calculations.rb @@ -13,16 +13,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 +49,9 @@ 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 } + self.to_a.sum(*args) {|*block_args| yield(*block_args)} else calculate(:sum, *args) end diff --git a/activerecord/test/cases/calculations_test.rb b/activerecord/test/cases/calculations_test.rb index 65d28ea028240..bf8c0f1e70db6 100644 --- a/activerecord/test/cases/calculations_test.rb +++ b/activerecord/test/cases/calculations_test.rb @@ -383,22 +383,6 @@ 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), From ad9983f625ea414c5b7bbc0fd12be17ec04fc590 Mon Sep 17 00:00:00 2001 From: Carlos Antonio da Silva Date: Sat, 17 Nov 2012 19:08:07 -0200 Subject: [PATCH 2/3] Deprecate Relation#sum with a block. To perform a sum calculation over the array of elements, use to_a.sum(&block). Please check the discussion in f9cb645dfcb5cc89f59d2f8b58a019486c828c73 for more context. --- activerecord/CHANGELOG.md | 5 +++++ activerecord/lib/active_record/associations.rb | 2 +- activerecord/lib/active_record/relation/calculations.rb | 6 ++++++ .../test/cases/associations/has_many_associations_test.rb | 6 ++++++ activerecord/test/cases/calculations_test.rb | 6 ++++-- 5 files changed, 22 insertions(+), 3 deletions(-) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 22e57a6774f9d..e3ed3780dbdfe 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -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) diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 651b17920c39a..d8b6d7a86b57d 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -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 diff --git a/activerecord/lib/active_record/relation/calculations.rb b/activerecord/lib/active_record/relation/calculations.rb index 24f3c128d102d..72b035a0232e0 100644 --- a/activerecord/lib/active_record/relation/calculations.rb +++ b/activerecord/lib/active_record/relation/calculations.rb @@ -1,3 +1,5 @@ +require 'active_support/deprecation' + module ActiveRecord module Calculations # Count the records. @@ -51,6 +53,10 @@ def maximum(column_name, options = {}) # Person.sum('age') # => 4562 def sum(*args) if block_given? + 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) diff --git a/activerecord/test/cases/associations/has_many_associations_test.rb b/activerecord/test/cases/associations/has_many_associations_test.rb index 01afa087bea0c..6cdc166533974 100644 --- a/activerecord/test/cases/associations/has_many_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_associations_test.rb @@ -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 diff --git a/activerecord/test/cases/calculations_test.rb b/activerecord/test/cases/calculations_test.rb index bf8c0f1e70db6..5cb7eabf0ec30 100644 --- a/activerecord/test/cases/calculations_test.rb +++ b/activerecord/test/cases/calculations_test.rb @@ -389,8 +389,10 @@ def test_sum_with_from_option 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 From fd1c45761e023cecdc9ad472df64f8c265e25aaf Mon Sep 17 00:00:00 2001 From: Carlos Antonio da Silva Date: Mon, 19 Nov 2012 09:01:20 -0200 Subject: [PATCH 3/3] Remove the #sum method from CollectionAssociation Since edd94cee9af1688dd036fc58fd405adb30a5e0da, CollectionProxy delegates all calculation methods - except count - to the scope, which does basically what this method was doing, but since we're delegating from the proxy, the association method was never called. --- .../active_record/associations/collection_association.rb | 9 --------- 1 file changed, 9 deletions(-) diff --git a/activerecord/lib/active_record/associations/collection_association.rb b/activerecord/lib/active_record/associations/collection_association.rb index 862ff201debb1..1548e68cea733 100644 --- a/activerecord/lib/active_record/associations/collection_association.rb +++ b/activerecord/lib/active_record/associations/collection_association.rb @@ -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+.