Skip to content
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

Forward ActiveRecord::Relation#count to Enumerable#count if block given #24203

Merged
merged 1 commit into from May 16, 2016

Conversation

@sferik
Copy link
Contributor

sferik commented Mar 15, 2016

I came across this line of code today:

class Order
  has_many :deliveries

  def num_deliveries_in_progress
    deliveries.select { |delivery| delivery.in_progress? }.size
  end

end

I had the idea to refactor this to use Enumerable#count, which takes a block, instead of Enumerable#select followed by Array#size.

 class Order
   has_many :deliveries

   def num_deliveries_in_progress
+    deliveries.count { |delivery| delivery.in_progress? }
-    deliveries.select { |delivery| delivery.in_progress? }.size
   end

 end

What happened instead was this method always returned the count of all the deliveries for that order (even counting ones that are not in progress) because ActiveRecord::Relation#count silently discards the block argument.

This is a similar situation to ActiveRecord::Relation#find, which collides with Enumerable#find, however, in that case, super is called.

@rails-bot
Copy link

rails-bot commented Mar 15, 2016

r? @kaspth

(@rails-bot has picked a reviewer for you, use r? to override)

@sferik sferik force-pushed the sferik:count_with_block branch Mar 15, 2016
@sferik sferik changed the title Forward ActiveRecord::Relation#count to Enumerable#count Forward ActiveRecord::Relation#count to Enumerable#count if block given Mar 15, 2016
@sferik sferik force-pushed the sferik:count_with_block branch 2 times, most recently Mar 15, 2016
@kaspth
kaspth reviewed Mar 19, 2016
View changes
activerecord/lib/active_record/associations/collection_association.rb Outdated
@@ -249,6 +249,7 @@ def destroy_all
# Count all records using SQL. Construct options and pass them with
# scope to the target class's +count+.

This comment has been minimized.

Copy link
@kaspth

kaspth Mar 19, 2016

Member

Documentation is now outdated, we don't always count in SQL.

@@ -728,8 +728,8 @@ def distinct
# # #<Pet id: 2, name: "Spook", person_id: 1>,
# # #<Pet id: 3, name: "Choo-Choo", person_id: 1>
# # ]

This comment has been minimized.

Copy link
@kaspth

kaspth Mar 19, 2016

Member

This doc also says we're using SQL counting. We should also have an example with a block.

@kaspth
Copy link
Member

kaspth commented Mar 19, 2016

LGTM, but requires a doc update 😁 — ping me when you got 'em 😉

@sferik
Copy link
Contributor Author

sferik commented Mar 19, 2016

@kaspth Thank you for reviewing. I have added documentation for passing a block to ActiveRecord::Relation#count. Please let me know if there is anything else that needs to be done.

@kaspth
kaspth reviewed Mar 19, 2016
View changes
activerecord/lib/active_record/associations/collection_proxy.rb Outdated
#
# class Person < ActiveRecord::Base
# has_many :pets
# end
#
# person.pets.count # => 3
# # This will perform the count using SQL.

This comment has been minimized.

Copy link
@kaspth

kaspth Mar 19, 2016

Member

Think this would follow more logically by being moved just above.

@kaspth
kaspth reviewed Mar 19, 2016
View changes
activerecord/lib/active_record/associations/collection_proxy.rb Outdated
def count(column_name = nil)
@association.count(column_name)
# person.pets.count { |pet| pet.name.include?('-') } # => 2
# # Passing a block will select all pets in SQL and perform the count using Ruby.

This comment has been minimized.

Copy link
@kaspth

kaspth Mar 19, 2016

Member

Ditto here. We actually have a style for this type of doc:

# Passing a block will select all pets in SQL and perform the count using Ruby.
#
#   person.pets.count { ... } 

This comment has been minimized.

Copy link
@sferik

sferik Mar 19, 2016

Author Contributor

Okay. I was trying to follow the conventions of the project. If you look just below, there is an example of the explanatory comment going one line below the code example (line 747).

This comment has been minimized.

Copy link
@kaspth

kaspth Mar 19, 2016

Member

Interesting. Haven't seen that format used before to be honest. If we're going to follow file precedent, we should make it more "precedential": # executes something like <SQL query> and newlines.

@kaspth
Copy link
Member

kaspth commented Mar 19, 2016

Had some more comments about the docs. Remember to squash your commits 😁

@sgrif
Copy link
Contributor

sgrif commented Mar 19, 2016

Just as a reminder, we are way past the feature freeze for Rails 5. This shouldn't be merged until after 5 is branched from master.

@kaspth kaspth added this to the 5.1.0 milestone Mar 19, 2016
@sferik sferik force-pushed the sferik:count_with_block branch Mar 19, 2016
@sferik
Copy link
Contributor Author

sferik commented Mar 19, 2016

@kaspth I’ve fixed up the documentation as you’ve suggested and amended those changes to the previous commit. If you’d prefer, I can squash everything into a single commit.

@sferik
Copy link
Contributor Author

sferik commented Mar 19, 2016

@sgrif I didn’t realize the Rails 5 feature freeze was in effect but that makes sense, considering you’ve already started shipping betas. I’m a little disappointed this feature won’t make it into Rails 5.0 but I’m excited to have it be one of the first features merged into Rails 5.1! 😄

@kaspth
Copy link
Member

kaspth commented Mar 19, 2016

@sferik yeah, all the way down to one commit please.

I’m excited to have it be one of the first features merged into Rails 5.1!

You can bet your editor, it is! 😉

@sferik sferik force-pushed the sferik:count_with_block branch Mar 19, 2016
@sferik
Copy link
Contributor Author

sferik commented Mar 19, 2016

@kaspth Done.

@sferik sferik force-pushed the sferik:count_with_block branch to 5877239 Mar 19, 2016
@kaspth kaspth merged commit 8fffee4 into rails:master May 16, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kaspth
Copy link
Member

kaspth commented May 16, 2016

There we go, merged after master is 5.1 😁

@sferik sferik deleted the sferik:count_with_block branch May 16, 2016
kamipo added a commit to kamipo/rails that referenced this pull request Mar 10, 2017
Follow up of rails#24203.

Since b644964 `ActiveRecord::Relation` includes `Enumerable` so it is
enough to call `super` simply.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.