Layout/SpaceAroundOperators false positives after #7211

[bquorning]

After upgrading to RuboCop v0.74.0, I discovered some changes in what is disallowed by Layout/SpaceAroundOperators. git bisect shows that #7211 is the cause of these changes.

---

Expected behavior

For both of these examples, I would expect no offenses.

test_1.rb:

# frozen_string_literal: true

@x = 'One'
@foo    = 'Two'
@foobar = 'three'

test_2.rb:

# frozen_string_literal: true

@foobar1 = 'foo'
@foobar2 = 'foo'
@foobar3 = 'foo'

@x       = 'foo'

Actual behavior

Offenses:

test_1.rb:4:9: C: Layout/SpaceAroundOperators: Operator = should be surrounded by a single space.
@foo    = 'Two'
        ^

test_2.rb:7:10: C: Layout/SpaceAroundOperators: Operator = should be surrounded by a single space.
@x       = 'foo'
         ^

Steps to reproduce the problem

Run RuboCop on the code examples above.

RuboCop version

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

❯ ./exe/rubocop -V
0.74.0 (using Parser 2.6.3.0, running on ruby 2.6.3 x86_64-darwin18)
❯ git rev-parse @
d638fa4210155da6e2ad44370571a8b6c1bf3eb3

/cc @jfelchner

[jfelchner]

Thanks for pinging me @bquorning :)

Those examples you gave are considered errors. Each group of assignments now has its own alignment.

For the first one it should be pretty clear why that’s an error: You’ve told the ExtraSpacing cop that you want to align on equal signs and the equal signs aren’t aligned. In fact I believe that particular bit of code may have thrown the same error even before my PR.

For the second one, that code may not have thrown prior to my change but let me try to explain why it now does.

The ExtraSpacing cop now “groups” assignments. And it tries to do it intelligently. In most cases when you have multiple groups of assignments with empty lines between each group, you have a particular focus for what that group of assignments is doing. Like when you write and a single paragraph is focused on one particular idea.

By doing it this way, not only is it generally going to be what the user wants, but it gives the user the ability to reset the assignment alignment manually (by adding an empty line).

This is not perfect. Even in my own code there are places where it doesn’t do exactly the thing that I want. But those are edge cases. There are many cops that don’t do the thing you’d like every time. But the important thing is that it doesn’t do the wrong thing (like all the bugs the ExtraSpacing cop used to have) and that it keeps the bike shedding to a minimum.

For those times when it doesn’t do what you want, that’s what the disable cop directives are there for.

In the case of your examples, the solution is to add a blank line between the first and second line (for the first example) and to remove the blank line (in the second example).

[jfelchner]

@jonas054 if we start seeing a bunch of issues where people are confused as to why the empty line is resetting the alignment group, I may be open to adding an option to specify whether blank lines reset the alignment groups.

[bquorning]

Thanks for the clarification @jfelchner.

Regarding the first example, I was too focused on producing the minimal amount of code to reproduce the problem, and ended up with a bad example. This following example better illustrates why I thought it was a bug:

@x = 'One'
@y = 'One'
@z = 'One'
@foo    = 'Two'
@foobar = 'three'

which gives this RuboCop output:

Layout/SpaceAroundOperators: Operator = should be surrounded by a single space.
@foo    = 'Two'
        ^

And I agree with you (now that I know how the cop works) that the solution is to just add a blank line after @z.

There are many cops that don’t do the thing you’d like every time. But the important thing is that it doesn’t do the wrong thing

🙏 I agree.

[jfelchner]

@bquorning can you close this out please?