Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new InternalAffairs/MethodNameEndWith cop #10600

Conversation

nobuyo
Copy link
Contributor

@nobuyo nobuyo commented May 4, 2022

Added new cop inspired by #10594 (comment).

This cop checks potentially usage of method identifier predicates instead of method_name.to_s.end_with?.

example

# bad
node.method_name.to_s.end_with?('=')

# good
node.assignment_method?

# bad
node.method_name.to_s.end_with?('?')

# good
node.predicate_method?

# bad
node.method_name.to_s.end_with?('!')

# good
node.bang_method?

I also did some refactoring of existing cops to conform to this cop.


Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
  • Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

@nobuyo nobuyo force-pushed the add-new-internal-affairs-method-name-end-with-cop branch 2 times, most recently from 9040753 to 369ce69 Compare May 4, 2022 05:39
@nobuyo nobuyo marked this pull request as ready for review May 4, 2022 05:40
'!' => 'bang_method?',
'?' => 'predicate_method?'
}
end
Copy link
Member

@koic koic May 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using constant instead of method will reduce unnecessary object creation.

@@ -0,0 +1 @@
* [#10600](https://github.com/rubocop/rubocop/pull/10600): Add new `InternalAffairs/MethodNameEndWith` cop. ([@nobuyo][])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an internal private cop for development. So changelog entry may not be required.

@@ -83,6 +84,7 @@ def expected_name(method_name, prefix)
new_name << '?' unless method_name.end_with?('?')
new_name
end
# rubocop:enable InternalAffairs/MethodNameEndWith
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can probably narrow the scope of these disable comments. The positions will be clearer.

diff --git a/lib/rubocop/cop/naming/predicate_name.rb b/lib/rubocop/cop/naming/predicate_name.rb
index 740166125..fbbad0f26 100644
--- a/lib/rubocop/cop/naming/predicate_name.rb
+++ b/lib/rubocop/cop/naming/predicate_name.rb
@@ -66,12 +66,11 @@ module RuboCop

         private

-        # rubocop:todo InternalAffairs/MethodNameEndWith
         def allowed_method_name?(method_name, prefix)
           !(method_name.start_with?(prefix) && # cheap check to avoid allocating Regexp
               method_name.match?(/^#{prefix}[^0-9]/)) ||
             method_name == expected_name(method_name, prefix) ||
-            method_name.end_with?('=') ||
+            method_name.end_with?('=') || # rubocop:todo InternalAffairs/MethodNameEndWith
             allowed_method?(method_name)
         end

@@ -81,10 +80,10 @@ module RuboCop
                      else
                        method_name.dup
                      end
-          new_name << '?' unless method_name.end_with?('?')
+          new_name << '?' unless method_name.end_with?('?') # rubocop:todo InternalAffairs/MethodNameEndWith
+
           new_name
         end
-        # rubocop:enable InternalAffairs/MethodNameEndWith

         def message(method_name, new_name)
           "Rename `#{method_name}` to `#{new_name}`."

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And rubocop:disable may be preferable to rubocop:todo if there is no fix.

Copy link
Contributor Author

@nobuyo nobuyo May 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not impossible, but these methods are mostly concerned with method_name, and it seemed a bit off to change them to receive node as argument to conform to this cop.
I didn't know if I should refactor it here, so compromised with rubocop:todo.

@nobuyo
Copy link
Contributor Author

nobuyo commented May 5, 2022

Thank you so much! I'll start fixing them.

This cop checks potentially usage of method identifier predicates instead of `method_name.to_s.end_with?`.

```ruby
node.method_name.to_s.end_with?('=')

node.assignment_method?

node.method_name.to_s.end_with?('?')

node.predicate_method?

node.method_name.to_s.end_with?('!')

node.bang_method?
```

Use `call` instead of `{send csend}` in node matcher expression

Co-authored-by: Koichi ITO <koic.ito@gmail.com>

Tweak conditional method call for more DRY

Co-authored-by: Koichi ITO <koic.ito@gmail.com>
@nobuyo nobuyo force-pushed the add-new-internal-affairs-method-name-end-with-cop branch from 016a994 to 6eadcd9 Compare May 5, 2022 05:07
@nobuyo
Copy link
Contributor Author

nobuyo commented May 5, 2022

I've fixed and updated except for #10600 (comment).

@koic koic merged commit 73dfcf2 into rubocop:master May 5, 2022
@koic
Copy link
Member

koic commented May 5, 2022

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants