Add allowed method for Lint/Debugger #8929
Conversation
|
You should also update the docs and the default configuration. And remove the hardcoded methods. |
mfbmina
force-pushed
the
allowed_methods_on_lint_debugger
branch
from
5837b74
to
5ac9f11
Compare
|
@bbatsov please check again! |
config/default.yml
Outdated
| @@ -1415,6 +1415,18 @@ Lint/Debugger: | |||
| Enabled: true | |||
| VersionAdded: '0.14' | |||
| VersionChanged: '0.49' | |||
| AllowedMethods: | |||
There was a problem hiding this comment.
this seems the opposite of Allowed, should this be DisallowedMethods? ReportedMethods? DebuggerMethods? just Methods?
There was a problem hiding this comment.
Yep, you're toally right. I like the name DebuggerMethods best, although I'm also fine with "ForbiddenMethods".
There was a problem hiding this comment.
I also felt like this when implementing... shall we go with DebuggerMethods then?
Yout still have replace the hardcoded node patterns with something that's generated dynamically based on the cop's configuration. |
My solution for that was adding a The only edge case that I couldn't cover and had to add a special check when was |
|
@bbatsov @robotdana what you guys think? |
|
I think we're pretty close to merging this. I left a couple of small remarks. You'll also need to update the cop description to mention the configuration options and the rebase on top of master. |
mfbmina
force-pushed
the
allowed_methods_on_lint_debugger
branch
from
a039407
to
12bd068
Compare
lib/rubocop/cop/lint/debugger.rb
Outdated
| !!node.receiver | ||
| end | ||
|
|
||
| def special_rule?(node) |
There was a problem hiding this comment.
I don't get what's the purpose of this special_rule? method.
There was a problem hiding this comment.
this is an edge case that I couldn't find a better fix. Maybe you can help me:
Even tho Kernel is a receiver and save_and_open_page is a debugger method together they should not be an offense.
Here is the test case for it: https://github.com/rubocop-hq/rubocop/blob/12bd06882eaca12bd33c77f31e8d976bc3ad0bed/spec/rubocop/cop/lint/debugger_spec.rb#L164-L166
There was a problem hiding this comment.
I'd just drop this test case. For such a small thing people should add a local exclusion rule. It just doesn't fit with having with our goal to have a configurable cop.
There was a problem hiding this comment.
Cool! I will drop it! It was introduced here. 83902a6
mfbmina
force-pushed
the
allowed_methods_on_lint_debugger
branch
from
12bd068
to
e96be77
Compare
mfbmina
force-pushed
the
allowed_methods_on_lint_debugger
branch
from
7e99b5a
to
f548669
Compare
…vers for debug methods lead to offenses. Prior to rubocop#8929, `Lint/Debugger` looked for a specific receiver/method pair, but that PR changed it so that any of the `DebuggerReceivers` would apply, *or also* no receiver. This caused regressions (rubocop#7636 and rubocop#9500, for instance) because method names specified as debugger by this cop would now register offenses when called with no receiver. In order to fix it, I changed the configuration for `Lint/Debugger` a bit, but in a way that should not cause any major incompatibility. The allow list is now specified including its receiver so that the receiver can be checked. Existing configurations that just pass in a method name will be treated as expecting no receiver (there's a slight incompatibility there because previously `foo` would be matched by `Kernel.foo` but I think this is ok (and is resolvable by adding `Kernel.foo` to the configuration). I've also grouped the debugger methods by source (it's often confusing to me where each method comes from), which also allows an end-user to disable a pre-defined group of methods by specifying `Byebug: ~` for example. Finally I've deprecated the `DebuggerReceivers` configuration as it is no longer useful.
…r debug methods lead to offenses. Prior to #8929, `Lint/Debugger` looked for a specific receiver/method pair, but that PR changed it so that any of the `DebuggerReceivers` would apply, *or also* no receiver. This caused regressions (#7636 and #9500, for instance) because method names specified as debugger by this cop would now register offenses when called with no receiver. In order to fix it, I changed the configuration for `Lint/Debugger` a bit, but in a way that should not cause any major incompatibility. The allow list is now specified including its receiver so that the receiver can be checked. Existing configurations that just pass in a method name will be treated as expecting no receiver (there's a slight incompatibility there because previously `foo` would be matched by `Kernel.foo` but I think this is ok (and is resolvable by adding `Kernel.foo` to the configuration). I've also grouped the debugger methods by source (it's often confusing to me where each method comes from), which also allows an end-user to disable a pre-defined group of methods by specifying `Byebug: ~` for example. Finally I've deprecated the `DebuggerReceivers` configuration as it is no longer useful.
This PR fixes #8927.
It allows the user to add AllowedMethods for Lint/Debugger cop.