ExtraSpacing reform

[jonas054]

Based on @jacob-carlborg's question about detecting alignment in #1888, I have a suggestion for how we can change Style/ExtraSpacing so it can be enabled by default without generating a lot of false positives for common alignment idioms.

We allow a token preceded by multiple spaces if either of these is true:

1. there's a token on an adjacent line (previous or next), in the same column, also preceded by space
2. the token's first character is also present on an adjacent line in the same column
3. the token is an assignment operator, e.g. += and the = is aligned with an = on an adjacent line
4. the token is a comment and it's aligned with the previous or next comment

#1
a  = 1  if b == 2
aa = 10 unless bb == 20

#2
list             .select { |x| x > 3 }
(list - unwanted).select { |x| x.even? }

#3
a ||= 1
a  += b

#4
if a < 10
  add_more # just in case
end
take_half  # only fair

As is hinted in example 1, an enabled ExtraSpacing would also check space before modifier keywords, rendering #2027 unnecessary.

[bquorning]

I think 1, 3 and 4 look good.

Number 2 I don't understand – would you mind explaining in a bit more detail, or perhaps give a second example?

[jonas054]

2 is based on code I found in activesupport-3.2.19/lib/active_support/core_ext/date/calculations.rb:

      y, m = (year * 12 + (mon - 1) + n).divmod(12)
      m,   = (m + 1)                    .divmod(1)

I thought it looked reasonable to want to align like that, so I made a rule that admittedly is quite forgiving, erring on the side of caution so to speak.

[bbatsov]

Seems to me things might spiral of control if we head down this path. But if there's strong demand for something like this to be supported, I can live with it. Frankly, I think most of the alignment techniques serve more to obscure the code, rather than make it more clear, but everyone has their own opinion regarding this. 1 & 2 look pretty absurd to me.

[bquorning]

I misunderstood 1 (hadn’t noticed that if and unless were aligned). I would probably only enable 3, perhaps also 4.

[jonas054]

@bquorning My basic idea was that in order to bring the ExtraSpacing cop into the ranks of default-enabled cops, we need it to allow a pretty wide range of cases where extra spacing is used for alignment. If it only allows the most common cases that everybody agrees on, then it reports and auto-corrects code where people have spent time and effort to align words just to make the code more readable in their opinion.

For example, in our own lib/rubocop/cop/style/indentation_width.rb, by keeping only rules 3 and 4, we would have to change

          case rhs.type
          when :if            then on_if(rhs, base)
          when :while, :until then on_while(rhs, base)
          else                     return
          end

into

          case rhs.type
          when :if then on_if(rhs, base)
          when :while, :until then on_while(rhs, base)
          else return
          end

We have a few of these kinds of constructs.

So in my view, we either try to make the cop focus entirely on apparent mistakes and let it be enabled by default, or we leave things as they are.

[rrosenblum]

I like the idea of enabling this by default. Some of the supported spacings seem weird to be, but I can see why people would chose that alignment.

My biggest annoyance with custom alignment is its inconsistencies. Most of the time that I come across it, it seems to inconsistent throughout the file or section of code.

1 seems odd to me because it feels only partially aligned. I would prefer it without the extra space. If someone wants the extra space though, I think it should be

a  = 1  if     b  == 2
aa = 10 unless bb == 20

[jonas054]

@rrosenblum I agree. That does look better. But the point I'm trying to make is that ExtraSpacing should not care which one of those is better. It should recognize anything that could be an attempt at alignment and allow it, thereby limiting its reporting to mistakes, typos, cases of resting the thumb on the space bar for too long.

@bbatsov I think that by having those broad rules right from the start (e.g., rule 1 talks about any token being aligned), we avoid the risk of spiralling out of control (having to add more and more logic down the road).

[rrosenblum]

I cannot think of a use case that falls outside of your 4 examples. That should make this cop safe enough to enable by default.

[bbatsov]

@bbatsov I think that by having those broad rules right from the start (e.g., rule 1 talks about any token being aligned), we avoid the risk of spiralling out of control (having to add more and more logic down the road).

That one way to look into it - the other is that it becomes harder to enforce a particular style. I might accidentally do something that looks like an alignment and the check won't report it. This would solve one issue, but introduce others.

[jonas054]

That's an important aspect. People who feel that enforcing a consistent style is important, more important than allowing custom alignment, should be able to configure the cop to maintain the current behavior of reporting all extra spacing.

I don't think it's possible to implement a perfect check that finds all mistakes and still allows all kinds of reasonable alignment. What we can achieve is to give a choice between super strict and lenient. I think that's an improvement over today's implementation that's super strict and disabled by default. It's disabled in our own project, which means that the obvious mistakes (there are 16 of them that I've found) are still not fixed. That should motivate us to do something.

[bbatsov]

Fair enough. You have my blessing for the reform. :-)

[jonas054]

Thanks! I'm off to Denmark for some summer holiday activities now, and will be back in a week. I'll cook up a PR at that time.

[bbatsov]

No worries. Enjoy your vacation!

[jacob-carlborg]

@jonas054 Awesome, thanks.

[jfelchner]

@jonas054 I could kiss you for this. 😘 😀

Now if we could just get a cop to enforce and autocorrect alignment of tokens on consecutive lines, I would be in heaven.

foo ||= 'bar'
another_foo = 'another_bar'

Enforce alignment of = becomes:

foo       ||= 'bar'
another_foo = 'another_bar'

I appreciate all the hard work you and @bbatsov do. rubocop is truly one of the things that makes my development day better every single day. ❤️

[jonas054]

@jfelchner Thanks for the kind words. I think it's doable, but it would have to be implemented in a separate cop. We would have to solve the problem of the new cop and ExtraSpacing stepping on each others' toes, so to speak.

Please open a new issue for the enforced alignment functionality.