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

Implement a cop checking for ambiguous block association with a method #3931

Closed
bbatsov opened this issue Jan 18, 2017 · 2 comments
Closed

Comments

@bbatsov
Copy link
Collaborator

bbatsov commented Jan 18, 2017

Passing a single param to some method and a block (with no parens around the param) should yield a warning. Many people would confusingly believe the block would be associated with the first method, but it's actually going to be associated with its parameter.

See #3928 as a reference.

Here's an example:

def some_method(x)
  yield x
end

def bar(a)
  some_method a { puts 1 }
end

Expected behavior

This code should produce a warning saying you should use parens to disambiguate the meaning of your code.

Actual behavior

No warning is raised.

Steps to reproduce the problem

Just run RuboCop over the example code.

RuboCop version

$ rubocop -V
0.47.0 (using Parser 2.3.3.1, running on ruby 2.3.3 x86_64-linux)
smakagon added a commit to smakagon/rubocop that referenced this issue Mar 4, 2017
@smakagon
Copy link
Contributor

smakagon commented Mar 4, 2017

@bbatsov Sorry for spammy commits above.

I've created Pull Request. Thanks for review.
You're right, currently I have a lot of false positives there.

One thing that I'm not happy about now is that cop throws warnings for code like this:
expect{ order.expire }.not_to change{ order.unpublished_events }
allow(cop).to receive(:on_int) { raise RuntimeError } <-- travis is red because now rubocop's code doesn't pass own checks
environment ENV.fetch('RAILS_ENV') { 'development' }

What do you think about that? Should we catch those cases?
Or we should just catch ambiguous method call if a has no params, like this one:
some_method a { |foo| ... }

And don't catch these cases:
allow(cop).to receive(:on_int) { raise RuntimeError }
environment ENV.fetch('RAILS_ENV') { 'development' }

But in specs we still have code like this:
expect { order.expire }.not_to change { order.unpublished_events }

Which looks the same like problem described in feature request:
some_method a { |foo| ... }

Let me know what you think.

@smakagon
Copy link
Contributor

smakagon commented Mar 4, 2017

Refactored and Extended specs. Currently it catches cases like this:
some_method a { |foo| ... }
expect{ order.expire }.not_to change{ order.unpublished_events }

It will not catch method call with arguments but we can make it catch these cases easily, just let me know:
environment ENV.fetch('RAILS_ENV') { 'development' }
allow(cop).to receive(:on_int) { raise RuntimeError }

All tests pass. Travis-CI check is green.

@bbatsov bbatsov closed this as completed in 710d0e6 Mar 7, 2017
koppen added a commit to substancelab/dotfiles that referenced this issue May 26, 2017
Rubocop 0.48 introduced a new Lint/AmbiguousBlockAssocation cop [1], which
guards against the issues detailed in
rubocop/rubocop#3931.

However, the pattern is a solid RSpec idiom and not likely to cause the problems
Lint/AmbigiousBlockAssociation attempts to guard against. For example:

    expect { foo }.to change { bar }

so we'll just ignore it in RSpec as suggested in
rubocop/rubocop#4222.

[1]: https://github.com/bbatsov/rubocop/blob/d1b9d66c3518389b0c408a6a4a42061b36748df4/relnotes/v0.48.0.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants