Fix false positives in Rails/Pluck for non-AR/Array receivers#1628
Open
dfl wants to merge 2 commits into
Open
Conversation
Add an `AllowedReceivers` config option to `Rails/Pluck` that skips the
cop when any method in the receiver chain matches the list. Defaults to
Nokogiri traversal methods (`xpath`, `css`, `search`, `at_xpath`,
`at_css`) so that, e.g., `doc.xpath('//item').map { |n| n['name'] }` is
no longer autocorrected to a `pluck` call that the receiver does not
support.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds an
AllowedReceiversconfig option toRails/Pluckthat skips the cop when any method in the receiver's call chain matches the list. The default list includes the most common Nokogiri traversal methods (xpath,css,search,at_xpath,at_css).Motivation
Rails/Pluckrewrites.map { |el| el[:key] }to.pluck(:key)without inspecting the receiver. This produces broken autocorrections when the receiver isn't anActiveRecord::RelationorArray— for example, Nokogiri:The cop is documented as
@safety: unsafe, but Nokogiri is widespread enough that a default-on guard seems worth it. Other rubocop-rails cops (Rails/RedundantActiveRecordAllMethod,Rails/DynamicFindBy,Rails/SaveBang) already use anAllowedReceiversconfig for similar reasons.Behavior
AllowedReceiversis matched against anysend/csendnode anywhere in the receiver chain, by method name. This handles both immediate (page.css('.row').map { ... }) and chained (doc.xpath('//x').first_thing.map { ... }) receivers.[]on non-Array collections (HappyMapper, Mongoid documents, etc.) without further code changes.Changes
lib/rubocop/cop/rails/pluck.rb: addallowed_receiver?guard and early-return inon_block.config/default.yml: addAllowedReceiversdefault forRails/Pluck.spec/rubocop/cop/rails/pluck_spec.rb: 4 new examples covering immediate, chained, safe-nav, and a negative (AR chain still flagged).changelog/...: changelog entry.Test plan
bundle exec rspec spec/rubocop/cop/rails/pluck_spec.rb— 40/40 passingbundle exec rspec— full suite green (modulo the changelog PR-number format check, which will pass once the PR number is filled in)AllowedReceiversdefault list matches what the team is comfortable shipping by default