From 087e0ccb72e7a1491701dd1d1d49746f745d9d68 Mon Sep 17 00:00:00 2001 From: Daniel Colson Date: Tue, 17 Apr 2018 08:37:35 -0400 Subject: Add RuboCop for `assert_not` over `assert !` We added `assert_not` in f75addd "to replace warty 'assert !foo'". fa8d35b agrees that it is warty, and so do I. This custom Rubocop rule turns the wart into a violation. As with my last custom cop, https://github.com/rails/rails/pull/32441, I want to make sure this looks right on code climate before pushing another commit to autocorrect everything. @toshimaru I just noticed https://github.com/toshimaru/rubocop-rails/pull/26 Is there a better way to add these custom cops, or were you saying we shouldn't have custom cops at all? --- ci/custom_cops/lib/custom_cops.rb | 1 + ci/custom_cops/lib/custom_cops/assert_not.rb | 40 +++++++++++++++++++++ ci/custom_cops/test/custom_cops/assert_not_test.rb | 42 ++++++++++++++++++++++ ci/custom_cops/test/custom_cops/refute_not_test.rb | 11 +----- ci/custom_cops/test/support/cop_helper.rb | 12 +++++++ 5 files changed, 96 insertions(+), 10 deletions(-) create mode 100644 ci/custom_cops/lib/custom_cops/assert_not.rb create mode 100644 ci/custom_cops/test/custom_cops/assert_not_test.rb (limited to 'ci') diff --git a/ci/custom_cops/lib/custom_cops.rb b/ci/custom_cops/lib/custom_cops.rb index d5d17f8856..157b8247e4 100644 --- a/ci/custom_cops/lib/custom_cops.rb +++ b/ci/custom_cops/lib/custom_cops.rb @@ -1,3 +1,4 @@ # frozen_string_literal: true require_relative "custom_cops/refute_not" +require_relative "custom_cops/assert_not" diff --git a/ci/custom_cops/lib/custom_cops/assert_not.rb b/ci/custom_cops/lib/custom_cops/assert_not.rb new file mode 100644 index 0000000000..e722448e21 --- /dev/null +++ b/ci/custom_cops/lib/custom_cops/assert_not.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +module CustomCops + # Enforces the use of `assert_not` over `assert !`. + # + # @example + # # bad + # assert !x + # assert ! x + # + # # good + # assert_not x + # + class AssertNot < RuboCop::Cop::Cop + MSG = "Prefer `assert_not` over `assert !`" + + def_node_matcher :offensive?, "(send nil? :assert (send ... :!))" + + def on_send(node) + add_offense(node) if offensive?(node) + end + + def autocorrect(node) + expression = node.loc.expression + + ->(corrector) do + corrector.replace( + expression, + corrected_source(expression.source) + ) + end + end + + private + + def corrected_source(source) + source.gsub(/^assert(\(| ) *! */, "assert_not\\1") + end + end +end diff --git a/ci/custom_cops/test/custom_cops/assert_not_test.rb b/ci/custom_cops/test/custom_cops/assert_not_test.rb new file mode 100644 index 0000000000..abb151aeb4 --- /dev/null +++ b/ci/custom_cops/test/custom_cops/assert_not_test.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true + +require "support/cop_helper" +require_relative "../../lib/custom_cops/assert_not" + +class AssertNotTest < ActiveSupport::TestCase + include CopHelper + + setup do + @cop = CustomCops::AssertNot.new + end + + test "rejects 'assert !'" do + inspect_source @cop, "assert !x" + assert_offense @cop, "^^^^^^^^^ Prefer `assert_not` over `assert !`" + end + + test "rejects 'assert !' with a complex value" do + inspect_source @cop, "assert !a.b(c)" + assert_offense @cop, "^^^^^^^^^^^^^^ Prefer `assert_not` over `assert !`" + end + + test "autocorrects `assert !`" do + corrected = autocorrect_source(@cop, "assert !false") + assert_equal "assert_not false", corrected + end + + test "autocorrects `assert !` with extra spaces" do + corrected = autocorrect_source(@cop, "assert ! false") + assert_equal "assert_not false", corrected + end + + test "autocorrects `assert !` with parentheses" do + corrected = autocorrect_source(@cop, "assert(!false)") + assert_equal "assert_not(false)", corrected + end + + test "accepts `assert_not`" do + inspect_source @cop, "assert_not x" + assert_empty @cop.offenses + end +end diff --git a/ci/custom_cops/test/custom_cops/refute_not_test.rb b/ci/custom_cops/test/custom_cops/refute_not_test.rb index 5dbd8bf32a..f0f6eaeda0 100644 --- a/ci/custom_cops/test/custom_cops/refute_not_test.rb +++ b/ci/custom_cops/test/custom_cops/refute_not_test.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require "support/cop_helper" -require "./lib/custom_cops/refute_not" +require_relative "../../lib/custom_cops/refute_not" class RefuteNotTest < ActiveSupport::TestCase include CopHelper @@ -59,15 +59,6 @@ class RefuteNotTest < ActiveSupport::TestCase 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}`" diff --git a/ci/custom_cops/test/support/cop_helper.rb b/ci/custom_cops/test/support/cop_helper.rb index d259154df5..c2c6b969dd 100644 --- a/ci/custom_cops/test/support/cop_helper.rb +++ b/ci/custom_cops/test/support/cop_helper.rb @@ -16,6 +16,18 @@ module CopHelper rewrite(cop, processed_source) end + def assert_offense(cop, expected_message) + assert_not_empty( + cop.offenses, + "Expected offense with message \"#{expected_message}\", but got no offense" + ) + + offense = cop.offenses.first + carets = "^" * offense.column_length + + assert_equal expected_message, "#{carets} #{offense.message}" + end + private TARGET_RUBY_VERSION = 2.4 -- cgit v1.2.3