Regression of #4198 - Lint/AmbiguousBlockAssociation

[luke-hill]

---

Expected behavior

expect { move_direction }.not_to change { current_room_id } should not raise any offenses
expect { move_direction }.to change { current_room_id }.to(new_room_id) should not raise any offenses

Actual behavior

expect { move_direction }.not_to change { current_room_id } raises an offense
expect { move_direction }.to change { current_room_id }.to(new_room_id) does not raise an offense

Steps to reproduce the problem

Code is attached above. I added the working variety to show comparison.

RuboCop version

Include the output of rubocop -V or bundle exec rubocop -V if using Bundler. Here's an example:

$ [bundle exec] rubocop -V
0.76.0 (using Parser 2.6.5.0, running on ruby 2.6.3 x86_64-linux)

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution and understanding!

[mvz]

I'm still seeing this issue in RuboCop 0.83.0.

[kreintjes]

Still an issue in 0.91.0.

[kreintjes]

@luke-hill @mvz apparently it is desired behaviour, because the syntax is ambiguous. You can fix it as suggested here: #8732 (comment)

In some cases it might also be possible to write the matches as change(object, :attribute) instead of using a block, as suggested here: #4198 (comment)

Or you can disable the cop in this specific case as suggested by bbatsov here: #4198 (comment)

[luke-hill]

4198 is impossible here. As I don't have a method on my object output, just the object.

The issue is more than not_to is behaving different to to

[mvz]

This seems to be the same behaviour as mentioned in #4198 (comment). I'm not sure if that was actually ever fixed?

As an aside, maybe change(self, :current_room_id) works?

[reedstonefood]

If you're on rubocop 1.13 or later, then you can use IgnoreMethods (as mentioned just above this comment) to deal with this scenario. #9685

[jimbali]

Can't you just put parantheses around the change method to make it unambiguous?

expect { move_direction }.not_to(change { current_room_id })

[luke-hill]

Yep that was offered last year by @kreintjes

If someone from the rubocop technical team says this won't be fix then they can close it. I don't think the original method was that bad?

[jimbali]

ah ok, so you're just saying that it's not ambiguous? I was thinking that maybe it was somehow ambiguous in the ruby syntax and that putting parentheses makes it explicitly a method call rather than something else, but I'm not sure what else it would be. It seems like perfectly readable Rspec tbf.

[reedstonefood]

Our example of this is similar.

expect { subject.perform }.to raise_error { |error|
  # some expectation of error
  # another expectation of error
}

This could be fixed as suggested by @jimbali :

expect { subject.perform }.to(raise_error { |error|
  # some expectation of error
  # another expectation of error
})

However, IMO this reduces readability, which I find to be a fantastic feature of rspec. I don't want extra paranthesis above what is there already. We'll be using the IgnoreMethods functionality to stop these being flagged up.