End alignment with chained assignments

[cschramm]

With

a = b = c do

the EndAlignment cop expects end to be aligned with b, while in my opinion it should be a.

[bbatsov]

Yep, that's a bug - I guess @edzhelyov will take a look at it soon.

[jonas054]

@edzhelyov If you're busy with the AST traversal changes I can take a look at this. Just let me know.

[edzhelyov]

@jonas054 I discussed with @bbatsov some time ago, that the block strategy used by most editors is to just align the end with the start of the line where the do is defined. All the edge cases we saw, including this one will be fixed if we implement the same logic. I haven't checked how this could be implemented, it's was easier for me to check for the case when the the do is on the next line.

I'd like if you check and tell me your opinion about this.

[cschramm]

Expecting the end to be aligned with whatever's first in the line with the do was what I thought of first, too.

No edge case where that does not make sense came to my mind, yet. So I think that's probably the best solution for the cop.

[jonas054]

@edzhelyov Since our solution must cover the case where there is a line break after variable = I don't see how checking for the start of the line we do is would be any easier. So I think we're better off staying with the current algorithm and fixing edge cases one by one. Of course I could be missing something.

There are, by the way, other edge cases that we don't cover at the moment, for example

a(b do
  end)

at least if you go by how emacs indents it.

/tmp/edge.rb:2:3: W: end at 2, 2 is not aligned with a(b do at 1, 0
  end)
  ^^^

And this is also an exception to the rule about alignment with the first position of the line where dois.

[edzhelyov]

That case:

a(b do
  end)

in my Vim the end is aligned with the beginning of the line. Seems that Vim and Emacs indent differently here, but I think we should come up with a single rule about indenting, that's why I propose that we handle this one as all other cases - indent the end with the a(.

@jonas054 Feel free to fix this issue any way you find appropriate. I will experiment with the idea of indenting based on the start of the line later this week.

[jonas054]

@edzhelyov Okay, I'll do my solution, then we can compare it to yours.

Just out of curiosity, how does your vim (settings) indent a parameter list without blocks? Is it:

a_function(b,
           c)

[bbatsov]

@jonas054's alignment suggestion seems more reasonable to me. I guess vim's indentation logic is flawed in that particular scenario.

[edzhelyov]

@jonas054 Vim indents in the same way as you've suggested. It's the block alignment that is always with the beginning on the line where do is defined.

[lee-dohm]

Do we get information from the parser about the line that a statement (rather than expression) begins on? It seems we want to put the end at the same indentation level as the line the statement begins on.

Also, shall we collect the various cases we want to cover and the "correct" way to indent them? So far, I've seen:

foo do
end

a = foo do
end

a = b = foo do
end

a =
  foo do
end

bar(foo do
end)

[jonas054]

@lee-dohm I'm with you except for the last one. If we add another parameter to that example, I think it looks a bit weird:

bar(foo do
end,
    baz)

so I say (again) that

bar(foo do
    end,
    baz)

is the way to go.

And to answer your question, yes we can get all the position information we need from Parser, but it's not as simple as just asking for the statement.

[cschramm]

@jonas054 in my opinion

bar(
    foo do
    end,
    baz
)

is the way to go. If you add another parameter on a separate line, the first one should be separated from the function call as well.

The compact version would be:

bar(foo do
end, baz)

Anyway, I think if there's a controversy about an edge case like this, rubocop should allow more than one way. It does not make sense to enforce a style only a fraction of people (and editors) agree on. The result would be the other fractions switching off the respective cop losing the benefits of the undisputed styles as well, which would make this great gem a little less useful overall.

[bbatsov]

@cschramm We cannot support every possible scenario - the alignment code is pretty complex even now :-) Anyways, this particular issue is now fixed.

[cschramm]

Thanks! 😃