Skip to content

Commit

Permalink
Add MethodDefineMacros option to Naming/PredicateName cop
Browse files Browse the repository at this point in the history
Follow up for #4855 (comment).

Feature
=======

This PR also add the `MethodDefineMacros` option to check on methods
dynamically defined to `Naming/PredicateName` cop.

The target that RuboCop checks by default is `define_method`. This also
applies to end user application code. The following is config/default.yml.

```yaml
Naming/PredicateName:
  MethodDefineMacros:
    - define_method
    - define_singleton_method
```

`def_node_matcher` is also used for internal affairs of RuboCop
itself. The following is config/.rubocop.yml.

```yaml
Naming/PredicateName:
  MethodDefineMacros:
    - define_method
    - define_singleton_method
    - def_node_matcher
```

The following is the effect applied to the before master branch (995315f) .

```console
% bundle exec rake internal_investigation
Running RuboCop...
Inspecting 1010 files

(snip)

Offenses:

lib/rubocop/cop/rails/has_many_or_has_one_dependent.rb:26:26: C: Rename
has_many_or_has_one_without_options? to many_or_has_one_without_options?.
        def_node_matcher :has_many_or_has_one_without_options?, <<-PATTERN
                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
lib/rubocop/cop/rails/has_many_or_has_one_dependent.rb:30:26: C: Rename
has_many_or_has_one_with_options? to many_or_has_one_with_options?.
        def_node_matcher :has_many_or_has_one_with_options?, <<-PATTERN
                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
lib/rubocop/cop/rails/has_many_or_has_one_dependent.rb:34:26: C: Rename
has_dependent? to dependent?.
        def_node_matcher :has_dependent?, <<-PATTERN
                         ^^^^^^^^^^^^^^^
lib/rubocop/cop/rails/has_many_or_has_one_dependent.rb:38:26: C: Rename
has_through? to through?.
        def_node_matcher :has_through?, <<-PATTERN
                         ^^^^^^^^^^^^^
lib/rubocop/cop/rails/not_null_column.rb:34:26: C: Rename has_default?
to default?.
        def_node_matcher :has_default?, <<-PATTERN
                         ^^^^^^^^^^^^^

1010 files inspected, 5 offenses detected
RuboCop failed!
```
  • Loading branch information
koic authored and bbatsov committed Oct 17, 2017
1 parent 3d63dca commit afbb06f
Show file tree
Hide file tree
Showing 6 changed files with 100 additions and 3 deletions.
7 changes: 7 additions & 0 deletions .rubocop.yml
Expand Up @@ -10,6 +10,13 @@ AllCops:
- 'tmp/**/*'
TargetRubyVersion: 2.1

Naming/PredicateName:
# Method define macros for dynamically generated method.
MethodDefineMacros:
- define_method
- define_singleton_method
- def_node_matcher

Style/FrozenStringLiteralComment:
EnforcedStyle: always

Expand Down
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -11,6 +11,7 @@
* [#4663](https://github.com/bbatsov/rubocop/issues/4663): Add new `Style/CommentedKeyword` cop. ([@donjar][])
* Add `IndentationWidth` configuration for `Layout/Tab` cop. ([@rrosenblum][])
* [#4854](https://github.com/bbatsov/rubocop/pull/4854): Add new `Lint/RegexpAsCondition` cop. ([@pocke][])
* [#4862](https://github.com/bbatsov/rubocop/pull/4862): Add `MethodDefineMacros` option to `Naming/PredicateName` cop. ([@koic][])

### Bug fixes

Expand Down
4 changes: 4 additions & 0 deletions config/default.yml
Expand Up @@ -638,6 +638,10 @@ Naming/PredicateName:
# should still be accepted
NameWhitelist:
- is_a?
# Method define macros for dynamically generated method.
MethodDefineMacros:
- define_method
- define_singleton_method
# Exclude Rspec specs because there is a strong convention to write spec
# helpers in the form of `have_something` or `be_something`.
Exclude:
Expand Down
37 changes: 34 additions & 3 deletions lib/rubocop/cop/naming/predicate_name.rb
Expand Up @@ -18,12 +18,33 @@ module Naming
# # good
# def value? ...
class PredicateName < Cop
def_node_matcher :dynamic_method_define, <<-PATTERN
(send nil? #method_define_macros
(sym $_)
...)
PATTERN

def on_send(node)
dynamic_method_define(node) do |method_name|
predicate_prefixes.each do |prefix|
next if allowed_method_name?(method_name.to_s, prefix)

add_offense(
node,
location: node.first_argument.loc.expression,
message: message(method_name,
expected_name(method_name.to_s, prefix))
)
end
end
end

def on_def(node)
predicate_prefixes.each do |prefix|
method_name = node.method_name.to_s
next unless method_name.start_with?(prefix)
next if method_name == expected_name(method_name, prefix)
next if predicate_whitelist.include?(method_name)

next if allowed_method_name?(method_name, prefix)

add_offense(
node,
location: :name,
Expand All @@ -35,6 +56,12 @@ def on_def(node)

private

def allowed_method_name?(method_name, prefix)
!method_name.start_with?(prefix) ||
method_name == expected_name(method_name, prefix) ||
predicate_whitelist.include?(method_name)
end

def expected_name(method_name, prefix)
new_name = if prefix_blacklist.include?(prefix)
method_name.sub(prefix, '')
Expand All @@ -60,6 +87,10 @@ def predicate_prefixes
def predicate_whitelist
cop_config['NameWhitelist']
end

def method_define_macros(macro_name)
cop_config['MethodDefineMacros'].include?(macro_name.to_s)
end
end
end
end
Expand Down
1 change: 1 addition & 0 deletions manual/cops_naming.md
Expand Up @@ -256,6 +256,7 @@ Attribute | Value
NamePrefix | is_, has_, have_
NamePrefixBlacklist | is_, has_, have_
NameWhitelist | is_a?
MethodDefineMacros | define_method, define_singleton_method
Exclude | spec/\*\*/\*
### References
Expand Down
53 changes: 53 additions & 0 deletions spec/rubocop/cop/naming/predicate_name_spec.rb
Expand Up @@ -73,4 +73,57 @@ def is_a?
RUBY
end
end

context 'with method definition macros' do
let(:cop_config) do
{ 'NamePrefix' => %w[is_], 'NamePrefixBlacklist' => %w[is_],
'MethodDefineMacros' => %w[define_method def_node_matcher] }
end

it 'registers an offense when using `define_method`' do
expect_offense(<<-RUBY.strip_indent)
define_method(:is_hello) do |method_name|
^^^^^^^^^ Rename `is_hello` to `hello?`.
method_name == 'hello'
end
RUBY
end

it 'registers an offense when using an internal affair macro' do
expect_offense(<<-RUBY.strip_indent)
def_node_matcher :is_hello, <<-PATTERN
^^^^^^^^^ Rename `is_hello` to `hello?`.
(send
(send nil? :method_name) :==
(str 'hello'))
PATTERN
RUBY
end
end

context 'without method definition macros' do
let(:cop_config) do
{ 'NamePrefix' => %w[is_], 'NamePrefixBlacklist' => %w[is_] }
end

it 'registers an offense when using `define_method`' do
expect_offense(<<-RUBY.strip_indent)
define_method(:is_hello) do |method_name|
^^^^^^^^^ Rename `is_hello` to `hello?`.
method_name == 'hello'
end
RUBY
end

it 'does not register any offenses when using an internal affair macro' do
expect_no_offenses(<<-RUBY.strip_indent)
def_node_matcher :is_hello, <<-PATTERN
^^^^^^^^^ Rename `is_hello` to `hello?`.
(send
(send nil? :method_name) :==
(str 'hello'))
PATTERN
RUBY
end
end
end

0 comments on commit afbb06f

Please sign in to comment.