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

[Fix #9684] Support IgnoredMethods option for Lint/AmbiguousBlockAssociation #9685

Conversation

gPrado
Copy link
Contributor

@gPrado gPrado commented Apr 7, 2021

This PR adds support for IgnoredMethods parameter in Lint/AmbiguousBlockAssociation cop to allow better DSL support.
It fixes #9684 and is also related with #7486.

@gPrado gPrado force-pushed the support_ignored_methods_options_for_lint_ambiguous_block_association branch from 6e171a6 to 6b12444 Compare April 7, 2021 22:56
@bbatsov
Copy link
Collaborator

bbatsov commented Apr 8, 2021

Two points from me:

  1. Generally it's a bit weird for Lint cops to have configuration - e.g. the built-in Ruby lint that this cop emulates will always give you a warning in this case and there's nothing one can do about this. You'll notice that extremely few Lint cops have any form of configuration what-so-ever.
  2. We've started to move away from "Ignored" and we prefer "Allowed" these days.

I was also under the impression that the built-in Ruby warning has discouraged this particular style of using RSpec and it's not as common as it used to be.

I will consider the suggested change once the PR is ready, but I'm not making any promises that it will be merged.

@gPrado gPrado force-pushed the support_ignored_methods_options_for_lint_ambiguous_block_association branch from 6b12444 to 94d0122 Compare April 9, 2021 20:44
@gPrado gPrado force-pushed the support_ignored_methods_options_for_lint_ambiguous_block_association branch from 94d0122 to 7e838aa Compare April 9, 2021 22:45
@gPrado
Copy link
Contributor Author

gPrado commented Apr 9, 2021

Thanks for your comments, @bbatsov

I understand your point regarding Lint cops, but the intention for this configuration is exactly to enable support for such specific DSL, when you have absolute knowledge of the ambiguity risks. In the issue I mentioned the change RSpec matcher, but the same offenses are also reported when using the satisfy matcher. Currently, the only alternative left is to disable the cop entirely for (e.g.) all RSpec files.

I don't think an Allowed rule would work better here, since this is a very general Lint cop.

What do you think about a stricter configuration?
e.g. Instead of an array with ignored methods (e.g. ["change"]), it could be configured with an array of ignored pair methods (e.g. [["to", "change"], ["to_not", "change"]]). Maybe, it would make the intention even clearer when configuring this cop.

@bbatsov
Copy link
Collaborator

bbatsov commented Apr 10, 2021

I don't think an Allowed rule would work better here, since this is a very general Lint cop.

I don't get this part - "AllowedMethods" and "IgnoredMethods" mean exactly the same. It's just a different way of phrasing this.

class AmbiguousBlockAssociation < Base
include IgnoredMethods
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I had forgotten the mixing was named like this. You can escape from legacy. :D

@bbatsov
Copy link
Collaborator

bbatsov commented Apr 10, 2021

What do you think about a stricter configuration?

I'd be down with this, as it's going to yield less false negatives, but it will add extra verbosity. It's hard to say how much of an impact it'd have in practice.

@bbatsov bbatsov merged commit 7296719 into rubocop:master Apr 15, 2021
@bbatsov
Copy link
Collaborator

bbatsov commented Apr 15, 2021

No one had anything else to add, so it seems we can merge this. 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.

Support IgnoredMethods option for Lint/AmbiguousBlockAssociation
2 participants