Fix RSpec/ChangeByZero with compound operators

[BrianHawley]

Previously, it didn't recognize compound expressions using & or |.
This led it to correct part of the expressions as regular code, but the
generated code was malformed.

Now it recognizes compound expressions using the operators.

---

Before submitting the PR make sure the following are checked:

• Feature branch is up-to-date with master (if not - rebase it).
• Squashed related commits together.
• Added tests.
• Updated documentation.
• Added an entry to the CHANGELOG.md if the new code introduces user-observable changes.
• The build (bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).

[BrianHawley]

@pirj you reviewed and merged the previous PRs for this cop. Care to take a look at this change?

[pirj]

Fantastic, thanks!

[BrianHawley]

@pirj I just thought of another bad correction. It changes this:

expect { foo }.not_to change { Foo.bar }.by(0)

to this:

expect { foo }.not_to change { Foo.bar }

when it should change to this:

expect { foo }.to change { Foo.bar }

Just added a fix for that.

[BrianHawley]

@pirj could you review again, and approve the workflows again?

[pirj]

RSpec wouldn't allow .not_to change { }.by(...) to my best knowledge.

[BrianHawley]

@pirj you're right, rspec complains about that now. RSpec version 3 added that complaint, but older versions allowed that. Do you still want me to change it? I don't know if rubocop-rspec supports rspec 2.x or earlier...

[BrianHawley]

@pirj see https://github.com/rspec/rspec-expectations/blob/main/Changelog.md#300beta2--2014-02-17 for details.
Looks like it wasn't supported in earlier versions either, but rspec didn't complain about it, it just ignored by. This cop change would fix that error, FWIW. Still, better than a bad correction.

[BrianHawley]

@pirj do you still want me to undo the not_to and to_not support? As it is, with that support, it changes code that rspec complains about to semantically equivalent code that it wouldn't complain about. It might help those unfortunates who still have to update to rspec 3 in this day and age.

An alternative would be to just not autocorrect in that case, because the old autocorrection was wrong. Then let rspec 3+ complain about it.

[pirj]

if rubocop-rspec supports rspec 2.x or earlier

It's a good question. I would say we don't, as probably those with RSpec 2.x are on early Ruby 2.0/2.1 which we don't support. Might be Ruby version update is an easier task than RSpec 2 -> 3 migration.

But not to be overly defensive with our code, I'd remove this, and wait for someone to complain about this deficiency. We can reconsider if this happens.

[BrianHawley]

@pirj requested change made. Care to review again?

[pirj]

Thanks!

[Darhazer]

Wow I haven't seens specs using that syntax. Do we need a cop to force using and and or over & and |

[pirj]

I don’t know. Those are aliases to or and and. I’d check existing codebases if they make use of them. Some may prefer it, and I personally kind of like how it looks like.

[Darhazer]

Some may prefer it, and I personally kind of like how it looks like.

That's why the configurable enforced styles are ;)

[BrianHawley]

The & and | operators are better to use sometimes because their precedence is a good match for the circumstances, and they don't get caught up in enforced parentheses (as I am subjected to at work) like .and and .or are. Main downside of them is you have to put the operator on the end of the line if you're splitting lines as in the examples, due to Ruby line splitting semantics; .and and .or can be moved to the next line, as in the examples.

If you make a cop to prevent their use, make sure that there's an option to enforce their use, or at least to disable it.

[pirj]

their precedence is a good match for the circumstances

My thought exactly. Do you have an indicative example of that?

.and and .or can be moved to the next line, as in the examples.

There's another reason - code indentation.

expect { action! }
  .to change { foo.count }
  .and change { bar.count }
  .to(3)

reads weird. But this is what RuboCop indentation cops would insist on, for consistency. Mostly because they have no awareness on the semantics, as those two to calls are entirely different.
As opposed to this,

expect { action! }
  .to change { foo.count } &
    change { bar.count }
      .to(3)

looks almost properly indented, and there's no confusion between the "runner" to and the change qualifier to.

The indentation remains consistent with more chained matchers:

expect { action! }
  .to change { foo.count } &
    change { bar.count }
      .to(3) &
    change { baz.count }
      .to(1)

With and/or style, RuboCop's Layout/MultilineMethodCallIndentation suggests:

    expect { action! }
      .to change { foo.count }
      .and change { bar.count }
      .to(3)
      .and change { baz.count }
      .to(1)

which is incredibly ugly an forces one to put all (sometimes lengthy) qualifiers onto the same line with the matcher.

[pirj]

My apologies to let the discussion drift. @BrianHawley you are really welcome to open an issue or a PR for the new cop, and to add some reasoning. I'll do my best to find some proof of usage of &/| in the wild. I confess knowing about this syntax, and being frustrated with and/or at times, and never giving &/| a shot. Might be the cop, even with the opposite enforced style could spread the word about such an alternative.

[pirj]

Thank you, @BrianHawley !