Fix Rubocop Rails issue: Rails/FindEach#12289
Fix Rubocop Rails issue: Rails/FindEach#12289dacook merged 1 commit intoopenfoodfoundation:masterfrom
Conversation
anthonyms
force-pushed
the
11482-fix-rubocop-rails-issue-find_each
branch
from
5cf049a to
f173575
Compare
rioug
left a comment
rioug
left a comment
There was a problem hiding this comment.
Nice one ! Thanks for your help 🙏
rioug
added
the
technical changes only
dacook
left a comment
dacook
left a comment
There was a problem hiding this comment.
Thank you!
I have a comment about a couple of the changes, which I think need changing.
| cancelled_orders = [] | ||
| editable_orders.where(id: @order_ids).find_each do |order| | ||
| order.send_cancellation_email = @send_cancellation_email | ||
| order.restock_items = @restock_items | ||
| order.cancel | ||
| cancelled_orders << order | ||
| end | ||
| cancelled_orders |
There was a problem hiding this comment.
It took me a few minutes to work out why this was needed: because each returns the array of records, but find_each doesn't retain an array, to avoid taking up too much memory.
Hmm, so then if we need to store the records in an array, there's no reason to use find_each here!
± Could you please change this one back to each with a rubocop:disable?
There was a problem hiding this comment.
Theres some (little) benefit in retaining the find_each since the processing will be done in batches. The cancelled_orders array will expand gradually rather than suddenly.
The little benefit is negated by the lack of readability.
Will use a rubocop:disable for this one.
| scoped_variants = [] | ||
|
|
||
| distributed_products.variants_relation. | ||
| includes(:default_price, :stock_locations, :product). | ||
| where(product_id: products). | ||
| each { |v| scoper.scope(v) } # Scope results with variant_overrides | ||
| find_each { |v| scoped_variants << scoper.scope(v) } # Scope results with variant_overrides | ||
|
|
||
| scoped_variants |
anthonyms
force-pushed
the
11482-fix-rubocop-rails-issue-find_each
branch
from
f173575 to
25e3f30
Compare
dacook
left a comment
dacook
left a comment
There was a problem hiding this comment.
Thank you, and bonus points for comments explaining why! 💯
4 participants
What? Why?
Closes #12290
Fix all violations of the Rails/FindEach cop.
Rubocop recommends using
find_eachin place ofeachwhenever the call is chained to awhere,allornot.This PR is one of many PR's addressing issue #11482.
What should we test?
N/A
Release notes
Fix all violations of the Rails/FindEach cop.
Changelog Category (reviewers may add a label for the release notes):
Dependencies
N/A
Documentation updates
N/A