From d7fd884ac6bd9906ad361d26a50a5de55cb5738e Mon Sep 17 00:00:00 2001 From: Tejas Bubane Date: Sat, 22 Feb 2020 23:28:22 +0530 Subject: [PATCH] [Fix #7739] Allow methods raising NotImplementedError in `Lint/UnusedMethodArgument` Closes #7739 --- CHANGELOG.md | 1 + config/default.yml | 3 +- .../cop/lint/unused_method_argument.rb | 38 ++++++-- manual/cops_lint.md | 27 +++++- .../cop/lint/unused_method_argument_spec.rb | 89 ++++++++++++++++++- 5 files changed, 149 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 58f9e775840..aaa03328311 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ * [#7331](https://github.com/rubocop-hq/rubocop/issues/7331): Add `forbidden` option to `Style/ModuleFunction` cop. ([@weh][]) * [#7699](https://github.com/rubocop-hq/rubocop/pull/7699): Add new `Lint/StructNewOverride` cop. ([@ybiquitous][]) * [#7809](https://github.com/rubocop-hq/rubocop/pull/7809): Add auto-correction for `Style/EndBlock` cop. ([@tejasbubane][]) +* [#7739](https://github.com/rubocop-hq/rubocop/pull/7739): Add `IgnoreNotImplementedMethods` configuration to `Lint/UnusedMethodArgument`. ([@tejasbubane][]) ### Bug fixes diff --git a/config/default.yml b/config/default.yml index bca34d578f7..a5e885c2044 100644 --- a/config/default.yml +++ b/config/default.yml @@ -1768,9 +1768,10 @@ Lint/UnusedMethodArgument: StyleGuide: '#underscore-unused-vars' Enabled: true VersionAdded: '0.21' - VersionChanged: '0.35' + VersionChanged: '0.81' AllowUnusedKeywordArguments: false IgnoreEmptyMethods: true + IgnoreNotImplementedMethods: true Lint/UriEscapeUnescape: Description: >- diff --git a/lib/rubocop/cop/lint/unused_method_argument.rb b/lib/rubocop/cop/lint/unused_method_argument.rb index 346c9c10c6c..eb2a5e9e43d 100644 --- a/lib/rubocop/cop/lint/unused_method_argument.rb +++ b/lib/rubocop/cop/lint/unused_method_argument.rb @@ -38,9 +38,34 @@ module Lint # def do_something(unused) # end # + # @example IgnoreNotImplementedMethods: true (default) + # # good + # def do_something(unused) + # raise NotImplementedError + # end + # + # def do_something_else(unused) + # fail "TODO" + # end + # + # @example IgnoreNotImplementedMethods: false + # # bad + # def do_something(unused) + # raise NotImplementedError + # end + # + # def do_something_else(unused) + # fail "TODO" + # end + # class UnusedMethodArgument < Cop include UnusedArgument + def_node_matcher :not_implemented?, <<~PATTERN + {(send nil? :raise (const nil? :NotImplementedError)) + (send nil? :fail ...)} + PATTERN + def autocorrect(node) UnusedArgCorrector.correct(processed_source, node) end @@ -51,16 +76,17 @@ def check_argument(variable) return unless variable.method_argument? return if variable.keyword_argument? && cop_config['AllowUnusedKeywordArguments'] - - if cop_config['IgnoreEmptyMethods'] - body = variable.scope.node.body - - return if body.nil? - end + return if ignored_method?(variable.scope.node.body) super end + def ignored_method?(body) + cop_config['IgnoreEmptyMethods'] && body.nil? || + cop_config['IgnoreNotImplementedMethods'] && + not_implemented?(body) + end + def message(variable) message = +"Unused method argument - `#{variable.name}`." diff --git a/manual/cops_lint.md b/manual/cops_lint.md index 99fa2a7a1e7..2049f1a8cfa 100644 --- a/manual/cops_lint.md +++ b/manual/cops_lint.md @@ -2636,7 +2636,7 @@ AllowUnusedKeywordArguments | `false` | Boolean Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged --- | --- | --- | --- | --- -Enabled | Yes | Yes | 0.21 | 0.35 +Enabled | Yes | Yes | 0.21 | 0.81 This cop checks for unused method arguments. @@ -2683,6 +2683,30 @@ end def do_something(unused) end ``` +#### IgnoreNotImplementedMethods: true (default) + +```ruby +# good +def do_something(unused) + raise NotImplementedError +end + +def do_something_else(unused) + fail "TODO" +end +``` +#### IgnoreNotImplementedMethods: false + +```ruby +# bad +def do_something(unused) + raise NotImplementedError +end + +def do_something_else(unused) + fail "TODO" +end +``` ### Configurable attributes @@ -2690,6 +2714,7 @@ Name | Default value | Configurable values --- | --- | --- AllowUnusedKeywordArguments | `false` | Boolean IgnoreEmptyMethods | `true` | Boolean +IgnoreNotImplementedMethods | `true` | Boolean ### References diff --git a/spec/rubocop/cop/lint/unused_method_argument_spec.rb b/spec/rubocop/cop/lint/unused_method_argument_spec.rb index c0d0351991b..0c38209068b 100644 --- a/spec/rubocop/cop/lint/unused_method_argument_spec.rb +++ b/spec/rubocop/cop/lint/unused_method_argument_spec.rb @@ -4,7 +4,11 @@ subject(:cop) { described_class.new(config) } let(:cop_config) do - { 'AllowUnusedKeywordArguments' => false, 'IgnoreEmptyMethods' => false } + { + 'AllowUnusedKeywordArguments' => false, + 'IgnoreEmptyMethods' => false, + 'IgnoreNotImplementedMethods' => false + } end describe 'inspection' do @@ -434,4 +438,87 @@ def method(a, b, *others) RUBY end end + + context 'when IgnoreNotImplementedMethods config parameter is set' do + let(:cop_config) { { 'IgnoreNotImplementedMethods' => true } } + + it 'accepts a method with a single unused parameter & '\ + 'raises NotImplementedError' do + expect_no_offenses(<<~RUBY) + def method(arg) + raise NotImplementedError + end + RUBY + end + + it 'accepts a method with a single unused parameter & '\ + 'fails with message' do + expect_no_offenses(<<~RUBY) + def method(arg) + fail "TODO" + end + RUBY + end + + it 'accepts a method with a single unused parameter & '\ + 'fails without message' do + expect_no_offenses(<<~RUBY) + def method(arg) + fail + end + RUBY + end + + it 'accepts an empty singleton method with a single unused parameter &'\ + 'raise NotImplementedError' do + expect_no_offenses(<<~RUBY) + def self.method(unused) + raise NotImplementedError + end + RUBY + end + + it 'registers an offense for a non-empty method with a single unused ' \ + 'parameter' do + message = "Unused method argument - `arg`. If it's necessary, use " \ + '`_` or `_arg` as an argument name to indicate that it ' \ + "won't be used. You can also write as `method(*)` if you " \ + "want the method to accept any arguments but don't care " \ + 'about them.' + + expect_offense(<<~RUBY) + def method(arg) + ^^^ #{message} + 1 + end + RUBY + end + + it 'accepts an empty method with multiple unused parameters' do + expect_no_offenses(<<~RUBY) + def method(a, b, *others) + raise NotImplementedError + end + RUBY + end + + it 'registers an offense for a non-empty method with multiple unused ' \ + 'parameters' do + (a_message, b_message, others_message) = %w[a b others].map do |arg| + "Unused method argument - `#{arg}`. If it's necessary, use `_` or " \ + "`_#{arg}` as an argument name to indicate that it won't be used. " \ + 'You can also write as `method(*)` if you want the method ' \ + "to accept any arguments but don't care about them." + end + + expect_offense(<<~RUBY) + def method(a, b, *others) + ^^^^^^ #{others_message} + ^ #{b_message} + ^ #{a_message} + 1 + end + RUBY + end + end end