mirror of https://github.com/rails/rails
Add custom RuboCop for `assert_not` over `refute`
Since at leastcf4afc4
we have preferred `assert_not` methods over `refute` methods. I have seen plenty of comments in PRs about this, and we have tried to fix it a few times (5294ad8
,e45f176
,8910f12
,41f50be
,d4cfd54
,48a183e
, and211adb4
), but the `refute` methods keep sneaking back in. This custom RuboCop will take care of enforcing this preference, so we don't have to think about it again. I suspect there are other similar stylistic preferences that could be solved with some custom RuboCops, so I will definitely keep my eyes open. `assert_not` over `assert !` might be a good candidate, for example. I wasn't totally sure if `ci/custom_cops` was the best place to put this, but nothing else seemed quite right. At one point I had it set up as a gem, but I think custom cops like this would have limited value in another context. I want to see how code climate handles the new cops before autocorrecting the existing violations. If things go as expected, I will push another commit with those corrections.
This commit is contained in:
parent
09b2348f7f
commit
f88f5a457c
|
@ -1,3 +1,5 @@
|
|||
require: './ci/custom_cops/lib/custom_cops'
|
||||
|
||||
AllCops:
|
||||
TargetRubyVersion: 2.4
|
||||
# RuboCop has a bunch of cops enabled by default. This setting tells RuboCop
|
||||
|
@ -8,6 +10,11 @@ AllCops:
|
|||
- '**/vendor/**/*'
|
||||
- 'actionpack/lib/action_dispatch/journey/parser.rb'
|
||||
|
||||
# Prefer assert_not_x over refute_x
|
||||
CustomCops/RefuteNot:
|
||||
Include:
|
||||
- '**/*_test.rb'
|
||||
|
||||
# Prefer &&/|| over and/or.
|
||||
Style/AndOr:
|
||||
Enabled: true
|
||||
|
|
|
@ -0,0 +1,5 @@
|
|||
#!/usr/bin/env ruby
|
||||
# frozen_string_literal: true
|
||||
|
||||
COMPONENT_ROOT = File.expand_path("..", __dir__)
|
||||
require_relative "../../../tools/test"
|
|
@ -0,0 +1,3 @@
|
|||
# frozen_string_literal: true
|
||||
|
||||
require_relative "custom_cops/refute_not"
|
|
@ -0,0 +1,71 @@
|
|||
# frozen_string_literal: true
|
||||
|
||||
module CustomCops
|
||||
# Enforces the use of `#assert_not` methods over `#refute` methods.
|
||||
#
|
||||
# @example
|
||||
# # bad
|
||||
# refute false
|
||||
# refute_empty [1, 2, 3]
|
||||
# refute_equal true, false
|
||||
#
|
||||
# # good
|
||||
# assert_not false
|
||||
# assert_not_empty [1, 2, 3]
|
||||
# assert_not_equal true, false
|
||||
#
|
||||
class RefuteNot < RuboCop::Cop::Cop
|
||||
MSG = "Prefer `%<assert_method>s` over `%<refute_method>s`"
|
||||
|
||||
CORRECTIONS = {
|
||||
refute: "assert_not",
|
||||
refute_empty: "assert_not_empty",
|
||||
refute_equal: "assert_not_equal",
|
||||
refute_in_delta: "assert_not_in_delta",
|
||||
refute_in_epsilon: "assert_not_in_epsilon",
|
||||
refute_includes: "assert_not_includes",
|
||||
refute_instance_of: "assert_not_instance_of",
|
||||
refute_kind_of: "assert_not_kind_of",
|
||||
refute_nil: "assert_not_nil",
|
||||
refute_operator: "assert_not_operator",
|
||||
refute_predicate: "assert_not_predicate",
|
||||
refute_respond_to: "assert_not_respond_to",
|
||||
refute_same: "assert_not_same",
|
||||
refute_match: "assert_no_match"
|
||||
}.freeze
|
||||
|
||||
OFFENSIVE_METHODS = CORRECTIONS.keys.freeze
|
||||
|
||||
def_node_matcher :offensive?, "(send nil? #offensive_method? ...)"
|
||||
|
||||
def on_send(node)
|
||||
return unless offensive?(node)
|
||||
|
||||
message = offense_message(node.method_name)
|
||||
add_offense(node, location: :selector, message: message)
|
||||
end
|
||||
|
||||
def autocorrect(node)
|
||||
->(corrector) do
|
||||
corrector.replace(
|
||||
node.loc.selector,
|
||||
CORRECTIONS[node.method_name]
|
||||
)
|
||||
end
|
||||
end
|
||||
|
||||
private
|
||||
|
||||
def offensive_method?(method_name)
|
||||
OFFENSIVE_METHODS.include?(method_name)
|
||||
end
|
||||
|
||||
def offense_message(method_name)
|
||||
format(
|
||||
MSG,
|
||||
refute_method: method_name,
|
||||
assert_method: CORRECTIONS[method_name]
|
||||
)
|
||||
end
|
||||
end
|
||||
end
|
|
@ -0,0 +1,75 @@
|
|||
# frozen_string_literal: true
|
||||
|
||||
require "support/cop_helper"
|
||||
require "./lib/custom_cops/refute_not"
|
||||
|
||||
class RefuteNotTest < ActiveSupport::TestCase
|
||||
include CopHelper
|
||||
|
||||
setup do
|
||||
@cop = CustomCops::RefuteNot.new
|
||||
end
|
||||
|
||||
{
|
||||
refute: :assert_not,
|
||||
refute_empty: :assert_not_empty,
|
||||
refute_equal: :assert_not_equal,
|
||||
refute_in_delta: :assert_not_in_delta,
|
||||
refute_in_epsilon: :assert_not_in_epsilon,
|
||||
refute_includes: :assert_not_includes,
|
||||
refute_instance_of: :assert_not_instance_of,
|
||||
refute_kind_of: :assert_not_kind_of,
|
||||
refute_nil: :assert_not_nil,
|
||||
refute_operator: :assert_not_operator,
|
||||
refute_predicate: :assert_not_predicate,
|
||||
refute_respond_to: :assert_not_respond_to,
|
||||
refute_same: :assert_not_same,
|
||||
refute_match: :assert_no_match
|
||||
}.each do |refute_method, assert_method|
|
||||
test "rejects `#{refute_method}` with a single argument" do
|
||||
inspect_source(@cop, "#{refute_method} a")
|
||||
assert_offense @cop, offense_message(refute_method, assert_method)
|
||||
end
|
||||
|
||||
test "rejects `#{refute_method}` with multiple arguments" do
|
||||
inspect_source(@cop, "#{refute_method} a, b, c")
|
||||
assert_offense @cop, offense_message(refute_method, assert_method)
|
||||
end
|
||||
|
||||
test "autocorrects `#{refute_method}` with a single argument" do
|
||||
corrected = autocorrect_source(@cop, "#{refute_method} a")
|
||||
assert_equal "#{assert_method} a", corrected
|
||||
end
|
||||
|
||||
test "autocorrects `#{refute_method}` with multiple arguments" do
|
||||
corrected = autocorrect_source(@cop, "#{refute_method} a, b, c")
|
||||
assert_equal "#{assert_method} a, b, c", corrected
|
||||
end
|
||||
|
||||
test "accepts `#{assert_method}` with a single argument" do
|
||||
inspect_source(@cop, "#{assert_method} a")
|
||||
assert_empty @cop.offenses
|
||||
end
|
||||
|
||||
test "accepts `#{assert_method}` with multiple arguments" do
|
||||
inspect_source(@cop, "#{assert_method} a, b, c")
|
||||
assert_empty @cop.offenses
|
||||
end
|
||||
end
|
||||
|
||||
private
|
||||
|
||||
def assert_offense(cop, expected_message)
|
||||
assert_not_empty cop.offenses
|
||||
|
||||
offense = cop.offenses.first
|
||||
carets = "^" * offense.column_length
|
||||
|
||||
assert_equal expected_message, "#{carets} #{offense.message}"
|
||||
end
|
||||
|
||||
def offense_message(refute_method, assert_method)
|
||||
carets = "^" * refute_method.to_s.length
|
||||
"#{carets} Prefer `#{assert_method}` over `#{refute_method}`"
|
||||
end
|
||||
end
|
|
@ -0,0 +1,35 @@
|
|||
# frozen_string_literal: true
|
||||
|
||||
require "rubocop"
|
||||
|
||||
module CopHelper
|
||||
def inspect_source(cop, source)
|
||||
processed_source = parse_source(source)
|
||||
raise "Error parsing example code" unless processed_source.valid_syntax?
|
||||
investigate(cop, processed_source)
|
||||
processed_source
|
||||
end
|
||||
|
||||
def autocorrect_source(cop, source)
|
||||
cop.instance_variable_get(:@options)[:auto_correct] = true
|
||||
processed_source = inspect_source(cop, source)
|
||||
rewrite(cop, processed_source)
|
||||
end
|
||||
|
||||
private
|
||||
TARGET_RUBY_VERSION = 2.4
|
||||
|
||||
def parse_source(source)
|
||||
RuboCop::ProcessedSource.new(source, TARGET_RUBY_VERSION)
|
||||
end
|
||||
|
||||
def rewrite(cop, processed_source)
|
||||
RuboCop::Cop::Corrector.new(processed_source.buffer, cop.corrections)
|
||||
.rewrite
|
||||
end
|
||||
|
||||
def investigate(cop, processed_source)
|
||||
RuboCop::Cop::Commissioner.new([cop], [], raise_error: true)
|
||||
.investigate(processed_source)
|
||||
end
|
||||
end
|
Loading…
Reference in New Issue