Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Yield only one argument instead of splatting.

Add Changelog entry. Closes #4003
  • Loading branch information...
commit f9cb645dfcb5cc89f59d2f8b58a019486c828c73 1 parent 9cc2bf6
@carlosantoniodasilva carlosantoniodasilva authored
View
7 activerecord/CHANGELOG.md
@@ -1,5 +1,12 @@
## Rails 4.0.0 (unreleased) ##
+* Allow blocks for `count` with `ActiveRecord::Relation`, to work similar as
+ `Array#count`:
+
+ Person.where("age > 26").count { |person| gender == 'female' }

This should read Person.where("age > 26").count { |person| person.gender == 'female' }

Fixed c51fb02, thanks again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
+ *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.
View
4 activerecord/lib/active_record/relation/calculations.rb
@@ -21,7 +21,7 @@ module Calculations
# # => 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 { |*block_args| yield(*block_args) }
+ 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)
@@ -65,7 +65,7 @@ def maximum(column_name, options = {})
# # 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) { |*block_args| yield(*block_args) }
+ self.to_a.sum(*args) { |item| yield item }
else
calculate(:sum, *args)
end

15 comments on commit f9cb645

@splattael

This should read Person.where("age > 26").count { |person| person.gender == 'female' }

@adzap

I think the introduction of the count and sum with a block is an anti-pattern. The example usage is certainly better done as a where clause.

Perhaps the example could be reversed and the age is calculated from the DOB in the block, which is often a pain to do in the database. Either way it should point out that where possible, put such criteria in a where clause rather than a block.

@carlosantoniodasilva

I agree the example could be definitely improved yeah, docs could get better examples on this. In any case I think it's fine to have the block for count to work the same way as it does with arrays. I'll see how to improve docs a bit, thanks!

@adzap
@carlosantoniodasilva

Agreed that when used poorly, they can have a great cost. The same goes for batch processing and people not using find_each, for instance. People have to learn how to use the tools, and we can help by providing them and with better documentation - which hopefully I'll be able to improve the related docs soon. Thanks!

@adzap

That probably highlights why I disagree with this change the most. The find_each and find_in_batches only have one pathway of implementation, and do as advertised at a possible processing cost. However, count with a column name will do a relatively inexpensive SQL count, but count with a block will not do an SQL count at all, and will return perhaps a mountain of records.

Let's be clear that this only saves the developer from doing this

Model.select(...).all.count {|r| r.foo == 'bar' }
vs
Model.select(...).count {|r| r.foo == 'bar' }

Besides, the count behaviour is different enough from Array#count to not attempt unifying them. The argument to Array#count counts the number of appearances of that value. Model.count argument counts the presence of any value in the column.

Anyway, I've voiced my disagreement.

@carlosantoniodasilva

The argument difference exists because it was already an existent api of the calculation methods, probably not worthy to touch them. Same as sum with a block, so adding count comes to make the api as similar as possible in my opinion.

I've seen people using things like all.paginate all over the place in different applications, this comes at the same price as count {} or all.count {}, so people need to learn when to use them and when to avoid. In any case, it's more a matter of trying to maintain the api as similar as possible.

Thanks for your points on that, I'll try to bring someone's else attention to your comments.

@adzap

My arguments apply to sum as much count on this issue. I wasn't advocating changing them in AR, just that they are different to Array and so you shouldn't aim to unify them.

But

Model.all.paginate()
# is incorrect usage of
Model.paginate() 

and is not a non-SQL construct. Though

Model.count {}
# will be documented, and has the same higher cost as
Model.all.count {}

and is an abuse of a real SQL construct, which other pathway through the same method actually uses.

Ok, I'm done. Thanks.

@josevalim
Owner

I agree with @adzap. Moving count and sum to accept blocks and therefore work as an array is the wrong move. They are very different semantically, it is not worthy to unify them.

I think the original reason count and sum was accepted was because someone was passing a block and nothing was happening. In this case, I am even in favor of raising an exception in case someone gives a block. I am +1 to revert those changes (for count and sum).

Here is what is going to happen on real code:

1) I will have a function that calls count { ... } passing a block;
2) As soon as I pass a relation, with this patch, it will instantiate an array and loop over it. Everything works, but it is slow.
3) Without this patch, we could raise a nice exception. The developer will think, notice the error and say: actually, I could be smarter here and do this in my database, adding the proper indexes, etc. If the developer notices he cannot be smarter in that case, he can simply force a to_a call before calling count.

It is easy to say that "you just need to be careful", but it is not that simple, sometimes the count call is inside a library, a helper and you cannot really see what is happening.

@jonleighton
Collaborator

I was initially in favour of this but I think @josevalim makes a strong argument. I'm in favour of raising ArgumentError as he suggests.

@josevalim
Owner
@carlosantoniodasilva

@josevalim I'm going to work on that, to remove all related block functionality from array-like methods like count/sum, raising an exception instead (if a block is given).

@adzap with the above change we'll hopefully avoid possible mistakes, thanks for bringing this up.

Thanks guys!

@adzap

Thanks all.

@carlosantoniodasilva Thanks for the discussion and seeking feedback.

@carlosantoniodasilva I think the aggregate functions are the main offenders here. Anything that would cause a fan-out in records returned due to passing the block. Namely sum, count, min, max, and group, though I'm not sure which of these currently takes a block. The each method for example, is not one of these, but as kicker method on ARel, I'm sure it wasn't in danger.

@carlosantoniodasilva

@adzap I think that only count and sum can currently take a block, and count is only on master. Their block option is going to be removed and an exception raised instead. In any case, there's a great change that we'll be removing the block option from methods like find and select as well, to keep the api consistent. Thanks!

@carlosantoniodasilva

There it is a9aeba6, thanks everyone.

Please sign in to comment.
Something went wrong with that request. Please try again.