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 #4227] AmbiguousBlockAssociation False Positive #4228

Conversation

smakagon
Copy link
Contributor

@smakagon smakagon commented Apr 1, 2017

Fixes false positive of AmbiguousBlockAssociation cop reported in #4227

assert_equal site.posts.find { |p| p.title == "Foo Bar" }, results.first

@ashmaroli
Copy link

Thank you @smakagon for opening this PR. I tested this patch locally and found an additional false-alarm (which I had expected to be solved concurrently, so never included it in the issue ticket.)

While the following case is no longer flagged:

assert_equal site.posts.find { |p| p.title == "Foo Bar" }, results.first

The following is still flagged incorrectly...:

assert @site.pages.find { |page| page.data["date"] == date }
W: Parenthesize the param find to make sure that the block will be associated with the assert
method call.
      assert @site.pages.find { |page| page.data["date"] == date }
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

... and apparently, so is any call made by a receiver that is an instance_variable:

W: Parenthesize the param any? to make sure that the block will be associated with the assert
method call.
      assert @collection.docs.any? { |d| d.path.include?("all.dots") }
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
W: Parenthesize the param index to make sure that the block will be associated with the
refute_nil method call.
      refute_nil @site.posts.index { |post| ...
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

@smakagon
Copy link
Contributor Author

smakagon commented Apr 2, 2017

@ashmaroli
In your first example:

assert_equal site.posts.find { |p| p.title == "Foo Bar" }, results.first

You have 2 arguments for assert_equal function, so that block definitely belongs to find method and can not belong to assert_equal.

But with this example:
assert @site.pages.find { |page| page.data["date"] == date }

It's expected behavior because we can not say exactly if block belongs to find or assert. It's almost the same example that we had in description to initial issue:

some_method a { puts 1 }

For example Rspec users will get the same error for this code:
expect { order.expire }.to change { order.events }

If you don't want to add parentheses to those files It makes sense to disable this cop for tests/specs folder by adding this to .rubocop.yml file:

Lint/AmbiguousBlockAssociation:
  Exclude:
    - "spec/**/*"

@ashmaroli
Copy link

Thank you @smakagon for the prompt reply.
I tested again with corrections and indeed, the following is no longer flagged.

assert(@site.pages.find { |page| page.data["date"] == date })

👍

@bbatsov
Copy link
Collaborator

bbatsov commented Apr 2, 2017

Just one small thing - you forgot to add a changelog entry.

@smakagon smakagon force-pushed the 4227_ambiguous_block_association_false_positive branch from ea1cd5d to 783159b Compare April 2, 2017 13:33
@smakagon smakagon force-pushed the 4227_ambiguous_block_association_false_positive branch from 783159b to d32a3f5 Compare April 2, 2017 13:35
@smakagon
Copy link
Contributor Author

smakagon commented Apr 2, 2017

@bbatsov Thanks. Added Changelog entry.

@bbatsov bbatsov merged commit 6414db1 into rubocop:master Apr 2, 2017
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

3 participants