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
Add new Style/RedundantEach
cop
#11110
Conversation
44db13f
to
6576f79
Compare
I like the suggestion, but I'm wondering if this is not more of a Lint cop. |
# array.each.each_with_index { |v, i| do_something(v, i) } | ||
# | ||
# # good | ||
# array.each.with_index { |v, i| do_something(v, i) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing that using each_with_index
directly is also good. Same with each_with_object
. Probably we'll need some separate cop about such iterators (e.g. something suggesting each.with_object
over each_ with_object
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure,each_with_index
is also accepted. I've updated it!
OTOH, I did a quick check and saw no difference in performance. This autocorrection chooses each.with_object
because the receiver is an Enumerator
and using method chain. But, I'm not sure which of each_with_object
and each.with_object
is definitely better as the default when receiver is a collection object (e.g. instance of Array, Hash, Set).
Follow up https://github.com/rubocop/rubocop-rails/pull/812/files#r994033301. This PR adds new `Style/RedundantEach` cop that checks for redundant `each`. This cop is unsafe, as it can produce false positives if the receiver is not an `Enumerable`. ```ruby # bad array.each.each { |v| do_something(v) } # good array.each { |v| do_something(v) } # bad array.each.each_with_index { |v, i| do_something(v, i) } # good array.each.with_index { |v, i| do_something(v, i) } array.each_with_index { |v, i| do_something(v, i) } # bad array.each.each_with_object { |v, o| do_something(v, o) } # good array.each.with_object { |v, o| do_something(v, o) } array.each_with_object { |v, o| do_something(v, o) } ```
I was a little worried, but it's unsafe, so I chose Style over Lint. It's also because the original code is not a dangerous problem. For example, |
6576f79
to
73320cf
Compare
Thanks! |
Follow up https://github.com/rubocop/rubocop-rails/pull/812/files#r994033301.
This PR adds new
Style/RedundantEach
cop that checks for redundanteach
.This cop is unsafe, as it can produce false positives if the receiver is not an
Enumerable
.Before submitting the PR make sure the following are checked:
[Fix #issue-number]
(if the related issue exists).master
(if not - rebase it).bundle exec rake default
. It executes all tests and runs RuboCop on its own code.{change_type}_{change_description}.md
if the new code introduces user-observable changes. See changelog entry format for details.