Update Rails/Pluck to be aware of numblocks
#631
Merged
Conversation
This file contains 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
sammiya
force-pushed
the
pluck-numblocks
branch
from
4f3f4cd to
06dcac9
Compare
sammiya
changed the title
Rails/Pluck to be aware of numblocks.Rails/Pluck to be aware of numblocks
sammiya
force-pushed
the
pluck-numblocks
branch
from
06dcac9 to
f234517
Compare
|
I'm worried about the duplication of implementation. Can you integrate it as follows? diff --git a/lib/rubocop/cop/rails/pluck.rb b/lib/rubocop/cop/rails/pluck.rb
index 332e5ac94..a18b46032 100644
--- a/lib/rubocop/cop/rails/pluck.rb
+++ b/lib/rubocop/cop/rails/pluck.rb
@@ -18,28 +18,35 @@ module RuboCop
# Post.published.pluck(:title)
# [{ a: :b, c: :d }].pluck(:a)
class Pluck < Base
extend AutoCorrector
extend TargetRailsVersion
- MSG = 'Prefer `pluck(:%<value>s)` over `%<method>s { |%<argument>s| %<element>s[:%<value>s] }`.'
+ MSG = 'Prefer `pluck(:%<value>s)` over `%<current>s`.'
minimum_target_rails_version 5.0
def_node_matcher :pluck_candidate?, <<~PATTERN
- (block (send _ ${:map :collect}) (args (arg $_argument)) (send (lvar $_element) :[] (sym $_value)))
+ ({block numblock} (send _ ${:map :collect}) $_argument (send (lvar $_element) :[] (sym $_value)))
PATTERN
def on_block(node)
pluck_candidate?(node) do |method, argument, element, value|
- next unless argument == element
+ match = if node.block_type?
+ argument.children.first.source.to_sym == element
+ else # numblock
+ argument == 1 && element == :_1
+ end
+ next unless match
- message = message(method, argument, element, value)
+ message = message(value, node)
add_offense(offense_range(node), message: message) do |corrector|
corrector.replace(offense_range(node), "pluck(:#{value})")
end
end
end
+ alias on_numblock on_block
private
@@ -47,8 +54,10 @@ module RuboCop
node.send_node.loc.selector.join(node.loc.end)
end
- def message(method, argument, element, value)
- format(MSG, method: method, argument: argument, element: element, value: value)
+ def message(value, node)
+ current = node.send_node.loc.selector.join(node.loc.end).source
+
+ format(MSG, value: value, current: current)
end
end
end |
koic
reviewed
Jan 17, 2022
lib/rubocop/cop/rails/pluck.rb
Outdated
| @@ -13,6 +13,7 @@ module Rails | |||
| # # bad | |||
| # Post.published.map { |post| post[:title] } | |||
| # [{ a: :b, c: :d }].collect { |el| el[:a] } | |||
| # [{ a: :b, c: :d }].collect { _1[:a] } | |||
There was a problem hiding this comment.
It's a bit redundant and can be remove.
sammiya
force-pushed
the
pluck-numblocks
branch
from
f234517 to
17e81ea
Compare
|
@koic Thank you for the elegant suggestion! I've incorporated your code and made the following minor tweaks, so please let me know if anything you don't like:
diff --git a/lib/rubocop/cop/rails/pluck.rb b/lib/rubocop/cop/rails/pluck.rb
index 9218a135f..ffb42fb1f 100644
--- a/lib/rubocop/cop/rails/pluck.rb
+++ b/lib/rubocop/cop/rails/pluck.rb
@@ -26,11 +26,11 @@ module RuboCop
minimum_target_rails_version 5.0
def_node_matcher :pluck_candidate?, <<~PATTERN
- ({block numblock} (send _ ${:map :collect}) $_argument (send (lvar $_element) :[] (sym $_value)))
+ ({block numblock} (send _ {:map :collect}) $_argument (send (lvar $_element) :[] (sym $_value)))
PATTERN
def on_block(node)
- pluck_candidate?(node) do |method, argument, element, value|
+ pluck_candidate?(node) do |argument, element, value|
match = if node.block_type?
argument.children.first.source.to_sym == element
else # numblock
@@ -54,7 +54,7 @@ module RuboCop
end
def message(value, node)
- current = node.send_node.loc.selector.join(node.loc.end).source
+ current = offense_range(node).source
format(MSG, value: value, current: current)
end |
koic
reviewed
Jan 17, 2022
config/default.yml
Outdated
| @@ -547,6 +547,7 @@ Rails/Pluck: | |||
| StyleGuide: 'https://rails.rubystyle.guide#pluck' | |||
| Enabled: 'pending' | |||
| VersionAdded: '2.7' | |||
| VersionChanged: '<<next>>' | |||
There was a problem hiding this comment.
Can you remove it because the default configuration of this cop does not change.
Yeah, the adjustment looks good! |
sammiya
force-pushed
the
pluck-numblocks
branch
from
17e81ea to
ba199d6
Compare
|
Thanks! |
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.
Fixed
Rails/Pluckto catchmap { _1[:foo] }.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}.mdif the new code introduces user-observable changes. See changelog entry format for details.and description in grammatically correct, complete sentences.