Skip to content

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

Merged
kaspth merged 1 commit intorails:masterfrom
sferik:count_with_block
May 16, 2016
Merged

Forward ActiveRecord::Relation#count to Enumerable#count if block given#24203
kaspth merged 1 commit intorails:masterfrom
sferik:count_with_block

Conversation

@sferik
Copy link
Contributor

@sferik sferik commented Mar 15, 2016 •
edited
Loading

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 count_with_block branch from a2f123b to 38dde96 Compare March 15, 2016 05:32
@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 count_with_block branch 2 times, most recently from d934f34 to f8187aa Compare March 15, 2016 05:49
kaspth
kaspth reviewed Mar 19, 2016
View reviewed changes
activerecord/lib/active_record/associations/collection_association.rb Outdated
Copy link
Contributor

@kaspth kaspth Mar 19, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@kaspth
Copy link
Contributor

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 reviewed changes
activerecord/lib/active_record/associations/collection_proxy.rb Outdated
Copy link
Contributor

@kaspth kaspth Mar 19, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@kaspth
Copy link
Contributor

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 count_with_block branch from 8cba028 to 2bc79c9 Compare March 19, 2016 18:47
@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
Contributor

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 count_with_block branch from 2bc79c9 to 5c950d6 Compare March 19, 2016 19:03
@sferik
Copy link
Contributor Author

sferik commented Mar 19, 2016

@kaspth Done.

@sferik sferik force-pushed the count_with_block branch from 5c950d6 to 5877239 Compare March 19, 2016 22:29
@eval eval mentioned this pull request Mar 21, 2016
Closed
@sferik sferik mentioned this pull request Mar 28, 2016
Merged
@kaspth kaspth merged commit 8fffee4 into rails:master May 16, 2016
@kaspth
Copy link
Contributor

kaspth commented May 16, 2016

There we go, merged after master is 5.1 😁

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

Since b644964 `ActiveRecord::Relation` includes `Enumerable` so it is
enough to call `super` simply.
@kamipo kamipo mentioned this pull request Mar 10, 2017
Merged
@ashleyHutton ashleyHutton mentioned this pull request Jul 24, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@kaspth kaspth

Labels

activerecord

Projects

None yet

Milestone

5.1.0

Development

Successfully merging this pull request may close these issues.

5 participants

@sferik @rails-bot @kaspth @sgrif @maclover7