Add MethodDefineMacros option to Naming/PredicateName cop
#4862
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bbatsov
reviewed
Oct 15, 2017
| @@ -73,4 +73,57 @@ def is_a? | |||
| RUBY | |||
| end | |||
| end | |||
|
|
|||
| context 'with method define macros' do | |||
bbatsov
reviewed
Oct 15, 2017
| end | ||
| end | ||
|
|
||
| context 'without method define macros' do |
There was a problem hiding this comment.
Btw, it seems you copy/pasted the previous context here (or I'm missing something).
There was a problem hiding this comment.
It is written as with previous context, without here context.
Drenmi
reviewed
Oct 15, 2017
| @@ -638,6 +638,9 @@ Naming/PredicateName: | |||
| # should still be accepted | |||
| NameWhitelist: | |||
| - is_a? | |||
| # Method define macros for dynamically generated method. | |||
| MethodDefineMacros: | |||
| - define_method | |||
There was a problem hiding this comment.
Ruby also has #define_singleton_method. 🙂
Drenmi
reviewed
Oct 15, 2017
| PATTERN | ||
|
|
||
| def on_send(node) | ||
| dynamic_method_define(node) do |micro_name, method_name| |
Drenmi
reviewed
Oct 15, 2017
| @@ -18,12 +18,35 @@ module Naming | |||
| # # good | |||
| # def value? ... | |||
| class PredicateName < Cop | |||
| def_node_matcher :dynamic_method_define, <<-PATTERN | |||
There was a problem hiding this comment.
This is a bit fragmented. Use don it's own, this doesn't return what it says it does. It just returns an unfiltered list of methods. This could probably use a funcall (see the top level comment in NorePattern) to filter according to the actual whitelist.
There was a problem hiding this comment.
I made a fix using funcall with the following commit 🙏
8263598
Drenmi
reviewed
Oct 15, 2017
| @@ -35,6 +58,12 @@ def on_def(node) | |||
|
|
|||
| private | |||
|
|
|||
| def valid_method_name?(method_name, prefix) | |||
There was a problem hiding this comment.
Since Ruby has its own notion about "valid" method names, perhaps we can use other nomenclature here? Maybe something like #allowed_method_name?
koic
force-pushed
the
add_method_define_macros_to_naming_predicate_name
branch
4 times, most recently
from
e4dca40
to
8263598
Compare
|
Squash and rebase and we're good to go. |
koic
force-pushed
the
add_method_define_macros_to_naming_predicate_name
branch
from
8263598
to
8d68579
Compare
|
Thanks for the review. I rebased with squash. |
Follow up for rubocop#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! ```
koic
force-pushed
the
add_method_define_macros_to_naming_predicate_name
branch
from
8d68579
to
ae2c1ed
Compare
|
👍 |
koic
added a commit
to koic/rubocop
that referenced
this pull request
Oct 18, 2017
Follow up for rubocop#4862. As with `def_node_matcher`, this commit added `def_node_search` method used for internal affair. The following are the use cases. ```console % git grep def_node_search .rubocop.yml: - def_node_search lib/rubocop/cop/bundler/duplicated_gem.rb: def_node_search: gem_declarations, '(send nil? :gem str ...)' lib/rubocop/cop/bundler/ordered_gems.rb: def_node_search :gem_declarations, <<-PATTERN lib/rubocop/cop/internal_affairs/useless_message_assertion.rb: def_node_search :described_class_msg, <<-PATTERN lib/rubocop/cop/mixin/unused_argument.rb: def_node_search :uses_var?, '(lvar %)' lib/rubocop/cop/performance/redundant_block_call.rb: def_node_search :blockarg_calls, <<-PATTERN lib/rubocop/cop/performance/redundant_block_call.rb: def_node_search :blockarg_assigned?, <<-PATTERN lib/rubocop/cop/performance/regexp_match.rb: def_node_search :search_match_nodes, MATCH_NODE_PATTERN lib/rubocop/cop/performance/regexp_match.rb: def_node_search :last_matches, <<-PATTERN lib/rubocop/cop/rails/file_path.rb: def_node_search :rails_root_nodes?, <<-PATTERN lib/rubocop/cop/style/parallel_assignment.rb: def_node_search :uses_var?, '{({lvar ivar cvar gvar} %) (const _ %)}' lib/rubocop/cop/style/parallel_assignment.rb: def_node_search :matching_calls, '(send %1 %2 $...)' lib/rubocop/cop/style/signal_exception.rb: def_node_search :custom_fail_methods, lib/rubocop/node_pattern.rb: def def_node_search(method_name, pattern_str) ``` There are no offenses now, it will be set for the future.
11 tasks
bbatsov
pushed a commit
that referenced
this pull request
Oct 18, 2017
Follow up for #4862. As with `def_node_matcher`, this commit added `def_node_search` method used for internal affair. The following are the use cases. ```console % git grep def_node_search .rubocop.yml: - def_node_search lib/rubocop/cop/bundler/duplicated_gem.rb: def_node_search: gem_declarations, '(send nil? :gem str ...)' lib/rubocop/cop/bundler/ordered_gems.rb: def_node_search :gem_declarations, <<-PATTERN lib/rubocop/cop/internal_affairs/useless_message_assertion.rb: def_node_search :described_class_msg, <<-PATTERN lib/rubocop/cop/mixin/unused_argument.rb: def_node_search :uses_var?, '(lvar %)' lib/rubocop/cop/performance/redundant_block_call.rb: def_node_search :blockarg_calls, <<-PATTERN lib/rubocop/cop/performance/redundant_block_call.rb: def_node_search :blockarg_assigned?, <<-PATTERN lib/rubocop/cop/performance/regexp_match.rb: def_node_search :search_match_nodes, MATCH_NODE_PATTERN lib/rubocop/cop/performance/regexp_match.rb: def_node_search :last_matches, <<-PATTERN lib/rubocop/cop/rails/file_path.rb: def_node_search :rails_root_nodes?, <<-PATTERN lib/rubocop/cop/style/parallel_assignment.rb: def_node_search :uses_var?, '{({lvar ivar cvar gvar} %) (const _ %)}' lib/rubocop/cop/style/parallel_assignment.rb: def_node_search :matching_calls, '(send %1 %2 $...)' lib/rubocop/cop/style/signal_exception.rb: def_node_search :custom_fail_methods, lib/rubocop/node_pattern.rb: def def_node_search(method_name, pattern_str) ``` There are no offenses now, it will be set for the future.
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Follow up for #4855 (comment).
Cc @pocke who suggested this feature.
Feature
This PR also add the
MethodDefineMacrosoption to check on methods dynamically defined toNaming/PredicateNamecop.The target that RuboCop checks by default is
define_method. This also applies to end user application code. The following is config/default.yml.def_node_matcheris also used for internal affairs of RuboCop itself. The following is config/.rubocop.yml.The following is the effect applied to the the before master branch (995315f) .
Before submitting the PR make sure the following are checked:
[Fix #issue-number](if the related issue exists).master(if not - rebase it).rake spec) are passing.rake internal_investigation.and description in grammatically correct, complete sentences.
rake generate_cops_documentation(required only when you've added a new cop or changed the configuration/documentation of an existing cop).