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
Support autocorrect for Style/CaseEquality
cop
#8322
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,13 +29,20 @@ module Style | |
# (1..100).include?(7) | ||
# some_string =~ /something/ | ||
# | ||
class CaseEquality < Cop | ||
class CaseEquality < Base | ||
extend AutoCorrector | ||
|
||
MSG = 'Avoid the use of the case equality operator `===`.' | ||
|
||
def_node_matcher :case_equality?, '(send #const? :=== _)' | ||
def_node_matcher :case_equality?, '(send $#const? :=== $_)' | ||
|
||
def on_send(node) | ||
case_equality?(node) { add_offense(node, location: :selector) } | ||
case_equality?(node) do |lhs, rhs| | ||
add_offense(node.loc.selector) do |corrector| | ||
replacement = replacement(lhs, rhs) | ||
corrector.replace(node, replacement) if replacement | ||
end | ||
end | ||
end | ||
|
||
private | ||
|
@@ -47,6 +54,18 @@ def const?(node) | |
true | ||
end | ||
end | ||
|
||
def replacement(lhs, rhs) | ||
case lhs.type | ||
when :regexp | ||
"#{rhs.source} =~ #{lhs.source}" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Exactly! I've opened the PR #8527. |
||
when :begin | ||
child = lhs.children.first | ||
"#{lhs.source}.include?(#{rhs.source})" if child&.range_type? | ||
when :const | ||
"#{rhs.source}.is_a?(#{lhs.source})" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not all constants are class/module instances. Regexps are often bound to a constant, to be used with I think a better bet is to replace the expression only if the LHS constant is CamelCased. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for your feedback. I've opened the PR #8526. |
||
end | ||
end | ||
end | ||
end | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,43 +1,75 @@ | ||
# frozen_string_literal: true | ||
|
||
RSpec.describe RuboCop::Cop::Style::CaseEquality do | ||
subject(:cop) { described_class.new(config) } | ||
RSpec.describe RuboCop::Cop::Style::CaseEquality, :config do | ||
shared_examples 'offenses' do | ||
it 'does not fail when the receiver is implicit' do | ||
expect_no_offenses(<<~RUBY) | ||
puts "No offense" | ||
RUBY | ||
end | ||
|
||
let(:config) { RuboCop::Config.new } | ||
it 'registers an offense and corrects for === when the receiver is a regexp' do | ||
expect_offense(<<~RUBY) | ||
/OMG/ === var | ||
^^^ Avoid the use of the case equality operator `===`. | ||
RUBY | ||
|
||
it 'registers an offense for ===' do | ||
expect_offense(<<~RUBY) | ||
Array === var | ||
expect_correction(<<~RUBY) | ||
var =~ /OMG/ | ||
RUBY | ||
end | ||
|
||
it 'registers an offense and corrects for === when the receiver is a range' do | ||
expect_offense(<<~RUBY) | ||
(1..10) === var | ||
^^^ Avoid the use of the case equality operator `===`. | ||
RUBY | ||
|
||
expect_correction(<<~RUBY) | ||
(1..10).include?(var) | ||
RUBY | ||
end | ||
|
||
it 'registers an offense and does not correct for === when receiver is of some other type' do | ||
expect_offense(<<~RUBY) | ||
foo === var | ||
^^^ Avoid the use of the case equality operator `===`. | ||
RUBY | ||
RUBY | ||
|
||
expect_no_corrections | ||
end | ||
end | ||
|
||
context 'when constant checks are allowed' do | ||
let(:config) do | ||
RuboCop::Config.new( | ||
'Style/CaseEquality' => { | ||
'AllowOnConstant' => true | ||
} | ||
) | ||
context 'when AllowOnConstant is false' do | ||
let(:cop_config) do | ||
{ 'AllowOnConstant' => false } | ||
end | ||
|
||
it 'does not fail when the receiver is implicit' do | ||
expect_no_offenses(<<~RUBY) | ||
puts "No offense" | ||
it 'registers an offense and corrects for === when the receiver is a constant' do | ||
expect_offense(<<~RUBY) | ||
Array === var | ||
^^^ Avoid the use of the case equality operator `===`. | ||
RUBY | ||
|
||
expect_correction(<<~RUBY) | ||
var.is_a?(Array) | ||
RUBY | ||
end | ||
|
||
include_examples 'offenses' | ||
end | ||
|
||
context 'when AllowOnConstant is true' do | ||
let(:cop_config) do | ||
{ 'AllowOnConstant' => true } | ||
end | ||
|
||
it 'does not register an offense for === when the receiver is a constant' do | ||
expect_no_offenses(<<~RUBY) | ||
Array === var | ||
RUBY | ||
end | ||
|
||
it 'registers an offense for === when the receiver is not a constant' do | ||
expect_offense(<<~RUBY) | ||
/OMG/ === "OMG" | ||
^^^ Avoid the use of the case equality operator `===`. | ||
RUBY | ||
end | ||
include_examples 'offenses' | ||
end | ||
end |
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 think this should not be changed:
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.
The auto-correction has been added by the new API, I think it is okay to extend
Base
. Rather, changing the inheritance toCop
will fail tests.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.
@koic I might be a little confused between Base and Cop. I see most cops inherit from
Cop
, do notextend AutoCorrector
and have separate methodautocorrect
. Which of these is the preferred/new approach?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.
Inheriting from
Base
is preferred/new approach. Here's the doc: https://docs.rubocop.org/rubocop/v1_upgrade_notes.html