Naming/PredicateName: Optionally use Sorbet to detect predicate methods #13721
Conversation
- At the moment, the `Naming/PredicateName` cop checks for predicate methods based on the presence of a prefix in the method name. This is a common convention, but, for better or for worse, not all `has_` and `is_` methods, over time, return booleans. - So, I had an idea to use Sorbet's `sig` to detect predicate methods based on the return type. If a method has a `sig` with a return type of `T::Boolean`, and the method name does not end with `?`, then it needs one. - Add a `UseSorbetSigs` configuration option to the `Naming/PredicateName` cop. When set to `true`, the cop will check for predicate methods based on the presence of a Sorbet `sig` for the method with a return type of `T::Boolean` instead of based on method naming.
issyl0
force-pushed
the
naming-predicate-name-use-sorbet-types
branch
from
e917732 to
c7be641
Compare
|
I like this suggestion. Probably some other cops could benefit from a similar approach in the future. @koic @dvandersluis Any objections to this proposal? |
| @@ -100,6 +128,7 @@ def on_def(node) | |||
| method_name = node.method_name.to_s | |||
|
|
|||
| next if allowed_method_name?(method_name, prefix) | |||
| next if sorbet? && !boolean_sorbet_sig?(node) | |||
There was a problem hiding this comment.
What if a codebase uses sorbet partially? They might want to enable this option, but in that case, any files/defs that have not been marked with sig will not fall back to the default behaviour of the cop. I think we should probably use the existing behaviour if UseSorbetSigs is enabled, but the method has no sig.
There was a problem hiding this comment.
I thought about this as well, but probably it's not worth to overcomplicate things here, as if someone enables this they likely know what they are doing and the tradeoffs they are making.
But some mixed mode does make sense, so we can probably have two options for for UseSorbetSigs if we want to be more flexible here.
|
Thanks for the feedback! To set expectations, my weekend is pretty packed but I'll get back to this ASAP. ❤️ |
|
Since this cop handles dynamic methods (ie. |
Co-authored-by: Daniel Vandersluis <daniel.vandersluis@gmail.com>
|
Of course! Thanks for the continued follow-up, @dvandersluis. I've got time on |
|
I dealt with all of the comments (apart from the indeterminate one about mixed mode) and CI is green. 🎉 |
Naming/PredicateNamecop checks for predicate methods based on the presence of a prefix in the method name. This is a common convention, but, for better or for worse, not allhas_andis_methods, over time, return booleans.sigto detect predicate methods based on the return type. If a method has asigwith a return type ofT::Boolean, and the method name does not end with?, then it needs one.UseSorbetSigsconfiguration option to theNaming/PredicateNamecop. When set totrue, the cop will check for predicate methods based on the presence of a Sorbetsigfor the method with a return type ofT::Booleaninstead of based on method naming.Before submitting the PR make sure the following are checked:
Commit message starts with[Fix #issue-number](if the related issue exists).master(if not - rebase it).bundle exec rake default. It executes all tests and runs RuboCop on its own code.{change_type}_{change_description}.mdif the new code introduces user-observable changes. See changelog entry format for details.