Skip to content

Commit

Permalink
[Fix rubocop#2731] Customise method-creating methods.
Browse files Browse the repository at this point in the history
This addresses the issue raised in rubocop#2731 where methods (often from outside of Ruby itself) may be called within the context of a module that in turn define methods. The `Lint/UselessAccessModifier` wasn’t taking these into account.

Much like the existing approach taken with blocks/context-creating methods, there’s now a configuration option `MethodCreatingMethods` for the cop. This setting is an array of methods which, when called, are known to create other methods in the module’s current access context.
  • Loading branch information
pat committed Jan 9, 2017
1 parent 1121924 commit fc7f81b
Show file tree
Hide file tree
Showing 5 changed files with 74 additions and 1 deletion.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
* Renamed `MethodCallParentheses` to `MethodCallWithoutArgsParentheses`. ([@dominh][])
* [#3854](https://github.com/bbatsov/rubocop/pull/3854): Add new `Rails/ReversibleMigration` cop. ([@sue445][])
* [#3872](https://github.com/bbatsov/rubocop/pull/3872): Detect `String#%` with hash literal. ([@backus][])
* [#2731](https://github.com/bbatsov/rubocop/issues/2731): Allow configuration of method calls that create methods for `Lint/UselessAccessModifier`. ([@pat][])

### Changes

Expand Down Expand Up @@ -2584,3 +2585,4 @@
[@sue445]: https://github.com/sue445
[@zverok]: https://github.com/zverok
[@backus]: https://github.com/backus
[@pat]: https://github.com/pat
1 change: 1 addition & 0 deletions config/enabled.yml
Original file line number Diff line number Diff line change
Expand Up @@ -1279,6 +1279,7 @@ Lint/UselessAccessModifier:
Description: 'Checks for useless access modifiers.'
Enabled: true
ContextCreatingMethods: []
MethodCreatingMethods: []

Lint/UselessAssignment:
Description: 'Checks for useless assignment to a local variable.'
Expand Down
29 changes: 28 additions & 1 deletion lib/rubocop/cop/lint/useless_access_modifier.rb
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,18 @@ module Lint
# def some_other_private_method
# end
# end
#
# @example
# # Lint/UselessAccessModifier:
# # MethodCreatingMethods:
# # - delegate
# require 'active_support/core_ext/module/delegation'
# class Foo
# # this is not redundant because `delegate` creates methods
# private
#
# delegate :method_a, to: :method_b
# end
class UselessAccessModifier < Cop
MSG = 'Useless `%s` access modifier.'.freeze

Expand Down Expand Up @@ -170,7 +182,22 @@ def check_new_visibility(node, unused, new_vis, cur_vis)
end

def method_definition?(child)
static_method_definition?(child) || dynamic_method_definition?(child)
static_method_definition?(child) ||
dynamic_method_definition?(child) ||
any_method_definition?(child)
end

def any_method_definition?(child)
cop_config.fetch('MethodCreatingMethods', []).any? do |m|
matcher_name = "#{m}_method?".to_sym
unless respond_to?(matcher_name)
self.class.def_node_matcher matcher_name, <<-PATTERN
{def (send nil :#{m} ...)}
PATTERN
end

send(matcher_name, child)
end
end

def start_of_new_scope?(child)
Expand Down
13 changes: 13 additions & 0 deletions manual/cops_lint.md
Original file line number Diff line number Diff line change
Expand Up @@ -1784,12 +1784,25 @@ class Foo
end
end
```
```ruby
# Lint/UselessAccessModifier:
# MethodCreatingMethods:
# - delegate
require 'active_support/core_ext/module/delegation'
class Foo
# this is not redundant because `delegate` creates methods
private
delegate :method_a, to: :method_b
end
```

### Important attributes

Attribute | Value
--- | ---
ContextCreatingMethods |
MethodCreatingMethods |


## Lint/UselessAssignment
Expand Down
30 changes: 30 additions & 0 deletions spec/rubocop/cop/lint/useless_access_modifier_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,36 @@
end
end

context 'when using a known method-creating method' do
let(:config) do
RuboCop::Config.new(
'Lint/UselessAccessModifier' => {
'MethodCreatingMethods' => ['delegate']
}
)
end
it 'is aware that this creates a new method' do
src = ['class SomeClass',
' private',
' ',
' delegate :foo, to: :bar',
'end']
inspect_source(cop, src)
expect(cop.offenses).to be_empty
end

it 'still points out redundant uses within the module' do
src = ['class SomeClass',
' delegate :foo, to: :bar',
' ',
' private',
'end']
inspect_source(cop, src)
expect(cop.offenses.size).to eq(1)
expect(cop.offenses.first.line).to eq(4)
end
end

shared_examples 'at the top of the body' do |keyword|
it 'registers an offense for `public`' do
src = ["#{keyword} A",
Expand Down

0 comments on commit fc7f81b

Please sign in to comment.