From 13a2452aa433b3c41a7b69180045fdfba3e4489a Mon Sep 17 00:00:00 2001 From: Patrik Wehrli Date: Sat, 21 Mar 2020 15:19:04 +0100 Subject: [PATCH] [Fix #7331] Add 'forbidden' option to Style/ModuleFunction cop (#7451) Co-authored-by: Bozhidar Batsov --- CHANGELOG.md | 1 + config/default.yml | 1 + lib/rubocop/cop/style/module_function.rb | 66 ++++++++++++++++--- manual/cops_style.md | 29 +++++++- .../rubocop/cop/style/module_function_spec.rb | 65 ++++++++++++++++++ 5 files changed, 150 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0d7f53bb413..429eb679ca1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -72,6 +72,7 @@ * [#7641](https://github.com/rubocop-hq/rubocop/issues/7641): **(Breaking)** Remove `Style/BracesAroundHashParameters` cop. ([@pocke][]) * Add the method name to highlight area of `Layout/EmptyLineBetweenDefs` to help provide more context. ([@rrosenblum][]) * [#7652](https://github.com/rubocop-hq/rubocop/pull/7652): Allow `pp` to allowed names of `Naming/MethodParameterName` cop in default config. ([@masarakki][]) +* [#7331](https://github.com/rubocop-hq/rubocop/issues/7331): Add `forbidden` option to `Style/ModuleFunction` cop. ([@weh][]) * [#7309](https://github.com/rubocop-hq/rubocop/pull/7309): Mark `Lint/UselessSetterCall` an "not safe" and improve documentation. ([@jonas054][]) * [#7723](https://github.com/rubocop-hq/rubocop/pull/7723): Enable `Migration/DepartmentName` cop by default. ([@koic][]) diff --git a/config/default.yml b/config/default.yml index 319d338753f..aa85629be8d 100644 --- a/config/default.yml +++ b/config/default.yml @@ -3090,6 +3090,7 @@ Style/ModuleFunction: SupportedStyles: - module_function - extend_self + - forbidden Autocorrect: false SafeAutoCorrect: false diff --git a/lib/rubocop/cop/style/module_function.rb b/lib/rubocop/cop/style/module_function.rb index 5811100a9cf..401d1ab8532 100644 --- a/lib/rubocop/cop/style/module_function.rb +++ b/lib/rubocop/cop/style/module_function.rb @@ -6,7 +6,7 @@ module Style # This cop checks for use of `extend self` or `module_function` in a # module. # - # Supported styles are: module_function, extend_self. + # Supported styles are: module_function, extend_self, forbidden. # # @example EnforcedStyle: module_function (default) # # bad @@ -46,6 +46,29 @@ module Style # # ... # end # + # The option `forbidden` prohibits the usage of both styles. + # + # @example EnforcedStyle: forbidden + # # bad + # module Test + # module_function + # # ... + # end + # + # # bad + # module Test + # extend self + # # ... + # end + # + # # bad + # module Test + # extend self + # # ... + # private + # # ... + # end + # # These offenses are not safe to auto-correct since there are different # implications to each approach. class ModuleFunction < Cop @@ -55,6 +78,8 @@ class ModuleFunction < Cop 'Use `module_function` instead of `extend self`.' EXTEND_SELF_MSG = 'Use `extend self` instead of `module_function`.' + FORBIDDEN_MSG = + 'Do not use `module_function` or `extend self`.' def_node_matcher :module_function_node?, '(send nil? :module_function)' def_node_matcher :extend_self_node?, '(send nil? :extend self)' @@ -69,6 +94,8 @@ def on_module(node) end def autocorrect(node) + return if style == :forbidden + lambda do |corrector| if extend_self_node?(node) corrector.replace(node.source_range, 'module_function') @@ -80,22 +107,41 @@ def autocorrect(node) private - def each_wrong_style(nodes) + def each_wrong_style(nodes, &block) case style when :module_function - private_directive = nodes.any? { |node| private_directive?(node) } - - nodes.each do |node| - yield node if extend_self_node?(node) && !private_directive - end + check_module_function(nodes, &block) when :extend_self - nodes.each do |node| - yield node if module_function_node?(node) - end + check_extend_self(nodes, &block) + when :forbidden + check_forbidden(nodes, &block) + end + end + + def check_module_function(nodes) + private_directive = nodes.any? { |node| private_directive?(node) } + + nodes.each do |node| + yield node if extend_self_node?(node) && !private_directive + end + end + + def check_extend_self(nodes) + nodes.each do |node| + yield node if module_function_node?(node) + end + end + + def check_forbidden(nodes) + nodes.each do |node| + yield node if extend_self_node?(node) + yield node if module_function_node?(node) end end def message(_node) + return FORBIDDEN_MSG if style == :forbidden + style == :module_function ? MODULE_FUNCTION_MSG : EXTEND_SELF_MSG end end diff --git a/manual/cops_style.md b/manual/cops_style.md index 33b59b51f7c..aa7da79ad8e 100644 --- a/manual/cops_style.md +++ b/manual/cops_style.md @@ -3734,11 +3734,13 @@ Enabled | Yes | Yes (Unsafe) | 0.11 | 0.65 This cop checks for use of `extend self` or `module_function` in a module. -Supported styles are: module_function, extend_self. +Supported styles are: module_function, extend_self, forbidden. In case there are private methods, the cop won't be activated. Otherwise, it forces to change the flow of the default code. +The option `forbidden` prohibits the usage of both styles. + These offenses are not safe to auto-correct since there are different implications to each approach. @@ -3785,12 +3787,35 @@ module Test # ... end ``` +#### EnforcedStyle: forbidden + +```ruby +# bad +module Test + module_function + # ... +end + +# bad +module Test + extend self + # ... +end + +# bad +module Test + extend self + # ... + private + # ... +end +``` ### Configurable attributes Name | Default value | Configurable values --- | --- | --- -EnforcedStyle | `module_function` | `module_function`, `extend_self` +EnforcedStyle | `module_function` | `module_function`, `extend_self`, `forbidden` Autocorrect | `false` | Boolean ### References diff --git a/spec/rubocop/cop/style/module_function_spec.rb b/spec/rubocop/cop/style/module_function_spec.rb index 8eab6d4f1ab..5409f4606de 100644 --- a/spec/rubocop/cop/style/module_function_spec.rb +++ b/spec/rubocop/cop/style/module_function_spec.rb @@ -82,4 +82,69 @@ def test; end RUBY end end + + context 'when enforced style is `forbidden`' do + let(:cop_config) { { 'EnforcedStyle' => 'forbidden' } } + + context 'registers an offense for `extend self`' do + it 'in a module' do + expect_offense(<<~RUBY) + module Test + extend self + ^^^^^^^^^^^ Do not use `module_function` or `extend self`. + def test; end + end + RUBY + + expect_no_corrections + end + + it 'in a module with private methods' do + expect_offense(<<~RUBY) + module Test + extend self + ^^^^^^^^^^^ Do not use `module_function` or `extend self`. + def test; end + private + def test_private;end + end + RUBY + + expect_no_corrections + end + + it 'in a module with declarative private' do + expect_offense(<<~RUBY) + module Test + extend self + ^^^^^^^^^^^ Do not use `module_function` or `extend self`. + def test; end + private :test + end + RUBY + + expect_no_corrections + end + end + + it 'accepts `extend self` in a class' do + expect_no_offenses(<<~RUBY) + class Test + extend self + end + RUBY + end + + it 'registers an offense for `module_function` without an argument' do + expect_offense(<<~RUBY) + module Test + module_function + ^^^^^^^^^^^^^^^ Do not use `module_function` or `extend self`. + def test; end + end + RUBY + + expect_no_corrections + end + end end