Skip to content

Commit

Permalink
Tweak offense messages
Browse files Browse the repository at this point in the history
This PR tweaks offense messages. It has the following advantages:

- Compact offense messages are preferred
- These all have auto-corrections
- Implementation becomes simple

Also, since the removed message is equivalent to the highlight part,
there is no information lost.
  • Loading branch information
koic committed Mar 7, 2022
1 parent 8f25c50 commit b784f45
Show file tree
Hide file tree
Showing 32 changed files with 107 additions and 115 deletions.
3 changes: 1 addition & 2 deletions lib/rubocop/cop/minitest/assert_empty_literal.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,7 @@ class AssertEmptyLiteral < Base
include ArgumentRangeHelper
extend AutoCorrector

MSG = 'Prefer using `assert_empty(%<arguments>s)` over ' \
'`assert_equal(%<literal>s, %<arguments>s)`.'
MSG = 'Prefer using `assert_empty(%<arguments>s)`.'
RESTRICT_ON_SEND = %i[assert_equal].freeze

def_node_matcher :assert_equal_with_empty_literal, <<~PATTERN
Expand Down
4 changes: 2 additions & 2 deletions lib/rubocop/cop/minitest/assert_path_exists.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ module Minitest
class AssertPathExists < Base
extend AutoCorrector

MSG = 'Prefer using `%<good_method>s` over `%<bad_method>s`.'
MSG = 'Prefer using `%<good_method>s`.'
RESTRICT_ON_SEND = %i[assert].freeze

def_node_matcher :assert_file_exists, <<~PATTERN
Expand All @@ -32,7 +32,7 @@ def on_send(node)
assert_file_exists(node) do |path, failure_message|
failure_message = failure_message.first
good_method = build_good_method(path, failure_message)
message = format(MSG, good_method: good_method, bad_method: node.source)
message = format(MSG, good_method: good_method)

add_offense(node, message: message) do |corrector|
corrector.replace(node, good_method)
Expand Down
2 changes: 1 addition & 1 deletion lib/rubocop/cop/minitest/assert_silent.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ module Minitest
class AssertSilent < Base
extend AutoCorrector

MSG = 'Prefer using `assert_silent` over `assert_output("", "")`.'
MSG = 'Prefer using `assert_silent`.'

def_node_matcher :assert_silent_candidate?, <<~PATTERN
(block
Expand Down
3 changes: 1 addition & 2 deletions lib/rubocop/cop/minitest/assert_truthy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,7 @@ class AssertTruthy < Base
include ArgumentRangeHelper
extend AutoCorrector

MSG = 'Prefer using `assert(%<arguments>s)` over ' \
'`assert_equal(true, %<arguments>s)`.'
MSG = 'Prefer using `assert(%<arguments>s)`.'
RESTRICT_ON_SEND = %i[assert_equal].freeze

def_node_matcher :assert_equal_with_truthy, <<~PATTERN
Expand Down
20 changes: 9 additions & 11 deletions lib/rubocop/cop/minitest/refute_equal.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,20 +18,19 @@ class RefuteEqual < Base
include ArgumentRangeHelper
extend AutoCorrector

MSG = 'Prefer using `refute_equal(%<preferred>s)` over ' \
'`assert(%<over>s)`.'
MSG = 'Prefer using `refute_equal(%<preferred>s)`.'
RESTRICT_ON_SEND = %i[assert].freeze

def_node_matcher :assert_not_equal, <<~PATTERN
(send nil? :assert ${(send $_ :!= $_) (send (send $_ :! ) :== $_) } $... )
(send nil? :assert {(send $_ :!= $_) (send (send $_ :! ) :== $_) } $... )
PATTERN

def on_send(node)
preferred, over = process_not_equal(node)
return unless preferred && over
preferred = process_not_equal(node)
return unless preferred

assert_not_equal(node) do |_, expected, actual|
message = format(MSG, preferred: preferred, over: over)
assert_not_equal(node) do |expected, actual|
message = format(MSG, preferred: preferred)

add_offense(node, message: message) do |corrector|
corrector.replace(node.loc.selector, 'refute_equal')
Expand All @@ -54,11 +53,10 @@ def original_usage(first_part, custom_message)
end

def process_not_equal(node)
assert_not_equal(node) do |over, first_arg, second_arg, rest_args|
assert_not_equal(node) do |first_arg, second_arg, rest_args|
custom_message = rest_args.first
preferred = preferred_usage(first_arg, second_arg, custom_message)
over = original_usage(over.source, custom_message&.source)
return [preferred, over]

preferred_usage(first_arg, second_arg, custom_message)
end
end
end
Expand Down
4 changes: 2 additions & 2 deletions lib/rubocop/cop/minitest/refute_false.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ class RefuteFalse < Base
include ArgumentRangeHelper
extend AutoCorrector

MSG_FOR_ASSERT_EQUAL = 'Prefer using `refute(%<arguments>s)` over `assert_equal(false, %<arguments>s)`.'
MSG_FOR_ASSERT = 'Prefer using `refute(%<arguments>s)` over `assert(!%<arguments>s)`.'
MSG_FOR_ASSERT_EQUAL = 'Prefer using `refute(%<arguments>s)`.'
MSG_FOR_ASSERT = 'Prefer using `refute(%<arguments>s)`.'
RESTRICT_ON_SEND = %i[assert_equal assert].freeze

def_node_matcher :assert_equal_with_false, <<~PATTERN
Expand Down
4 changes: 2 additions & 2 deletions lib/rubocop/cop/minitest/refute_path_exists.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ module Minitest
class RefutePathExists < Base
extend AutoCorrector

MSG = 'Prefer using `%<good_method>s` over `%<bad_method>s`.'
MSG = 'Prefer using `%<good_method>s`.'
RESTRICT_ON_SEND = %i[refute].freeze

def_node_matcher :refute_file_exists, <<~PATTERN
Expand All @@ -32,7 +32,7 @@ def on_send(node)
refute_file_exists(node) do |path, failure_message|
failure_message = failure_message.first
good_method = build_good_method(path, failure_message)
message = format(MSG, good_method: good_method, bad_method: node.source)
message = format(MSG, good_method: good_method)

add_offense(node, message: message) do |corrector|
corrector.replace(node, good_method)
Expand Down
4 changes: 2 additions & 2 deletions lib/rubocop/cop/mixin/in_delta_mixin.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,15 @@ module RuboCop
module Cop
# Common functionality for `AssertInDelta` and `RefuteInDelta` cops.
module InDeltaMixin
MSG = 'Prefer using `%<good_method>s` over `%<bad_method>s`.'
MSG = 'Prefer using `%<good_method>s`.'

def on_send(node)
equal_floats_call(node) do |expected, actual, message|
message = message.first
good_method = build_good_method(expected, actual, message)

if expected.float_type? || actual.float_type?
message = format(MSG, good_method: good_method, bad_method: node.source)
message = format(MSG, good_method: good_method)

add_offense(node, message: message) do |corrector|
corrector.replace(node, good_method)
Expand Down
8 changes: 2 additions & 6 deletions lib/rubocop/cop/mixin/minitest_cop_rule.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,7 @@ def define_rule(assertion_method, target_method:, preferred_method: nil, inverse
include ArgumentRangeHelper
extend AutoCorrector
MSG = 'Prefer using `#{preferred_method}(%<new_arguments>s)` over ' \
'`#{assertion_method}(%<original_arguments>s)`.'
MSG = 'Prefer using `#{preferred_method}(%<new_arguments>s)`.'
RESTRICT_ON_SEND = %i[#{assertion_method}].freeze
def on_send(node)
Expand Down Expand Up @@ -70,12 +69,9 @@ def offense_message(arguments)
message_argument&.source
].flatten.compact.join(', ')
original_arguments = arguments.map(&:source).join(', ')
format(
MSG,
new_arguments: new_arguments,
original_arguments: original_arguments
new_arguments: new_arguments
)
end
Expand Down
6 changes: 3 additions & 3 deletions lib/rubocop/minitest/assert_offense.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ module Minitest
# class FooTest < Minitest::Test
# def test_do_something
# assert_equal(nil, somestuff)
# ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_nil(somestuff)` over `assert_equal(nil, somestuff)`.
# ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_nil(somestuff)`.
# end
# end
# RUBY
Expand All @@ -36,7 +36,7 @@ module Minitest
# class FooTest < Minitest::Test
# def test_do_something
# assert_equal(nil, somestuff)
# ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_nil(somestuff)` over `assert_equal(nil, somestuff)`.
# ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_nil(somestuff)`.
# end
# end
# RUBY
Expand Down Expand Up @@ -64,7 +64,7 @@ module Minitest
# class FooTest < Minitest::Test
# def test_do_something
# assert_equal(nil, somestuff)
# ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_nil(somestuff)` over `assert_equal(nil, somestuff)`.
# ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_nil(somestuff)`.
# end
# end
# RUBY
Expand Down
4 changes: 2 additions & 2 deletions test/rubocop/cop/minitest/assert_empty_literal_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ def test_registers_offense_when_asserting_empty_array
class FooTest < Minitest::Test
def test_do_something
assert_equal([], somestuff)
^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_empty(somestuff)` over `assert_equal([], somestuff)`.
^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_empty(somestuff)`.
end
end
RUBY
Expand All @@ -27,7 +27,7 @@ def test_registers_offense_when_asserting_empty_hash
class FooTest < Minitest::Test
def test_do_something
assert_equal({}, somestuff)
^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_empty(somestuff)` over `assert_equal({}, somestuff)`.
^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_empty(somestuff)`.
end
end
RUBY
Expand Down
8 changes: 4 additions & 4 deletions test/rubocop/cop/minitest/assert_empty_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ def test_registers_offense_when_using_assert_with_empty
class FooTest < Minitest::Test
def test_do_something
assert(somestuff.empty?)
^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_empty(somestuff)` over `assert(somestuff.empty?)`.
^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_empty(somestuff)`.
end
end
RUBY
Expand All @@ -27,7 +27,7 @@ def test_registers_offense_when_using_assert_with_empty_and_string_message
class FooTest < Minitest::Test
def test_do_something
assert(somestuff.empty?, 'message')
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_empty(somestuff, 'message')` over `assert(somestuff.empty?, 'message')`.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_empty(somestuff, 'message')`.
end
end
RUBY
Expand All @@ -46,7 +46,7 @@ def test_registers_offense_when_using_assert_with_empty_and_heredoc_message
class FooTest < Minitest::Test
def test_do_something
assert(somestuff.empty?, <<~MESSAGE
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_empty(somestuff, <<~MESSAGE)` over `assert(somestuff.empty?, <<~MESSAGE)`.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_empty(somestuff, <<~MESSAGE)`.
message
MESSAGE
)
Expand All @@ -71,7 +71,7 @@ def test_registers_offense_when_using_assert_with_empty_in_redundant_parentheses
class FooTest < Minitest::Test
def test_do_something
assert((somestuff.empty?))
^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_empty(somestuff)` over `assert(somestuff.empty?)`.
^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_empty(somestuff)`.
end
end
RUBY
Expand Down
12 changes: 6 additions & 6 deletions test/rubocop/cop/minitest/assert_equal_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ def test_registers_offense_when_using_assert_equal_operator_with_string
class FooTest < Minitest::Test
def test_do_something
assert('rubocop-minitest' == actual)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_equal('rubocop-minitest', actual)` over `assert('rubocop-minitest' == actual)`.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_equal('rubocop-minitest', actual)`.
end
end
RUBY
Expand All @@ -27,7 +27,7 @@ def test_registers_offense_when_using_assert_equal_operator_with_object
class FooTest < Minitest::Test
def test_do_something
assert(expected == actual)
^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_equal(expected, actual)` over `assert(expected == actual)`.
^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_equal(expected, actual)`.
end
end
RUBY
Expand All @@ -46,7 +46,7 @@ def test_registers_offense_when_using_assert_equal_operator_with_method_call
class FooTest < Minitest::Test
def test_do_something
assert(obj.expected == other_obj.actual)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_equal(obj.expected, other_obj.actual)` over `assert(obj.expected == other_obj.actual)`.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_equal(obj.expected, other_obj.actual)`.
end
end
RUBY
Expand All @@ -65,7 +65,7 @@ def test_registers_offense_when_using_assert_equal_operator_with_the_message
class FooTest < Minitest::Test
def test_do_something
assert('rubocop-minitest' == actual, 'message')
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_equal('rubocop-minitest', actual, 'message')` over `assert('rubocop-minitest' == actual, 'message')`.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_equal('rubocop-minitest', actual, 'message')`.
end
end
RUBY
Expand All @@ -84,7 +84,7 @@ def test_registers_offense_when_using_assert_equal_operator_with_heredoc_message
class FooTest < Minitest::Test
def test_do_something
assert('rubocop-minitest' == actual, <<~MESSAGE
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_equal('rubocop-minitest', actual, <<~MESSAGE)` over `assert('rubocop-minitest' == actual, <<~MESSAGE)`.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_equal('rubocop-minitest', actual, <<~MESSAGE)`.
message
MESSAGE
)
Expand All @@ -109,7 +109,7 @@ def test_registers_offense_when_using_assert_with_equal_in_redundant_parentheses
class FooTest < Minitest::Test
def test_do_something
assert(('rubocop-minitest' == actual))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_equal('rubocop-minitest', actual)` over `assert('rubocop-minitest' == actual)`.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_equal('rubocop-minitest', actual)`.
end
end
RUBY
Expand Down
6 changes: 3 additions & 3 deletions test/rubocop/cop/minitest/assert_in_delta_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ def test_registers_offense_when_using_assert_equal_with_float_as_expected_value
class FooTest < Minitest::Test
def test_do_something
assert_equal(0.2, foo)
^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_in_delta(0.2, foo)` over `assert_equal(0.2, foo)`.
^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_in_delta(0.2, foo)`.
end
end
RUBY
Expand All @@ -27,7 +27,7 @@ def test_registers_offense_when_using_assert_equal_with_float_as_actual_value
class FooTest < Minitest::Test
def test_do_something
assert_equal(foo, 0.2)
^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_in_delta(foo, 0.2)` over `assert_equal(foo, 0.2)`.
^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_in_delta(foo, 0.2)`.
end
end
RUBY
Expand All @@ -46,7 +46,7 @@ def test_registers_offense_when_using_assert_equal_with_float_and_message
class FooTest < Minitest::Test
def test_do_something
assert_equal(0.2, foo, 'message')
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_in_delta(0.2, foo, 0.001, 'message')` over `assert_equal(0.2, foo, 'message')`.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_in_delta(0.2, foo, 0.001, 'message')`.
end
end
RUBY
Expand Down
8 changes: 4 additions & 4 deletions test/rubocop/cop/minitest/assert_includes_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ def test_registers_offense_when_using_assert_with_include
class FooTest < Minitest::Test
def test_do_something
assert(collection.include?(object))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_includes(collection, object)` over `assert(collection.include?(object))`.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_includes(collection, object)`.
end
end
RUBY
Expand All @@ -27,7 +27,7 @@ def test_registers_offense_when_using_assert_with_include_and_message
class FooTest < Minitest::Test
def test_do_something
assert(collection.include?(object), 'message')
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_includes(collection, object, 'message')` over `assert(collection.include?(object), 'message')`.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_includes(collection, object, 'message')`.
end
end
RUBY
Expand All @@ -46,7 +46,7 @@ def test_registers_offense_when_using_assert_with_include_and_heredoc_message
class FooTest < Minitest::Test
def test_do_something
assert(collection.include?(object), <<~MESSAGE
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_includes(collection, object, <<~MESSAGE)` over `assert(collection.include?(object), <<~MESSAGE)`.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_includes(collection, object, <<~MESSAGE)`.
message
MESSAGE
)
Expand All @@ -71,7 +71,7 @@ def test_registers_offense_when_using_assert_with_include_in_redundant_parenthes
class FooTest < Minitest::Test
def test_do_something
assert((collection.include?(object)))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_includes(collection, object)` over `assert(collection.include?(object))`.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_includes(collection, object)`.
end
end
RUBY
Expand Down
8 changes: 4 additions & 4 deletions test/rubocop/cop/minitest/assert_instance_of_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ def test_registers_offense_when_using_assert_with_instance_of
class FooTest < Minitest::Test
def test_do_something
assert(object.instance_of?(SomeClass))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_instance_of(SomeClass, object)` over `assert(object.instance_of?(SomeClass))`.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_instance_of(SomeClass, object)`.
end
end
RUBY
Expand All @@ -27,7 +27,7 @@ def test_registers_offense_when_using_assert_with_instance_of_and_message
class FooTest < Minitest::Test
def test_do_something
assert(object.instance_of?(SomeClass), 'message')
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_instance_of(SomeClass, object, 'message')` over `assert(object.instance_of?(SomeClass), 'message')`.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_instance_of(SomeClass, object, 'message')`.
end
end
RUBY
Expand All @@ -46,7 +46,7 @@ def test_registers_offense_when_using_assert_instance_of_operator_with_heredoc_m
class FooTest < Minitest::Test
def test_do_something
assert(object.instance_of?(SomeClass), <<~MESSAGE
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_instance_of(SomeClass, object, <<~MESSAGE)` over `assert(object.instance_of?(SomeClass), <<~MESSAGE)`.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_instance_of(SomeClass, object, <<~MESSAGE)`.
message
MESSAGE
)
Expand All @@ -71,7 +71,7 @@ def test_registers_offense_when_using_assert_with_instance_of_in_redundant_paren
class FooTest < Minitest::Test
def test_do_something
assert((object.instance_of?(SomeClass)))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_instance_of(SomeClass, object)` over `assert(object.instance_of?(SomeClass))`.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_instance_of(SomeClass, object)`.
end
end
RUBY
Expand Down

0 comments on commit b784f45

Please sign in to comment.