Auto-correction of argument indentation bug

[bbatsov]

Code like this:

 create :transaction, :closed,
          account:          account,
          open_price:       1.29,
          close_price:      1.30

If auto-corrected to:

 create :transaction, :closed,
        account:          account,
          open_price:       1.29,
          close_price:      1.30

The second line's indentation is corrected, but others remain with an incorrect indentation.

[jonas054]

Here's how I've been thinking about this problem.

The key-value pairs that get misaligned are not individual parameters, so they really deserve a cop of their own that checks key alignment in multi-line hash literals. But they were aligned from the start, and the AlignParameters cop messed them up. So how do we solve it? A generic solution that inspects and corrects repeatedly until no more changes are needed, or a specific solution for just this problem implemented in AlignParameters?

Any opinions?

[bbatsov]

I think we should have a separate cop about the alignment of literals like [] and {} indeed. As for the solution of the current problem - I'm always in favour of generic solutions.

[jonas054]

I will start by adding cops for literal alignment. I will come back to the question of how to solve the problem of one autocorrection changing things so another correction is required. It looks very tricky.

But first I want to ask about code that we currently have in RuboCop, e.g.

        @output_hash = {
          metadata: metadata_hash,
             files: [],
           summary: { offence_count: 0 }
        }

This uses a symmetrical alignment around the colon. My question is, should we allow this? @yujinakayama @bbatsov ?

[yujinakayama]

I wrote it. :)
I guess there are three alignment styles for hash literal:

@output_hash = {
  metadata: metadata_hash,
     files: [],
   summary: { offence_count: 0 }
}

@output_hash = {
  metadata: metadata_hash,
  files:    [],
  summary:  { offence_count: 0 }
}

@output_hash = {
  metadata: metadata_hash,
  files: [],
  summary: { offence_count: 0 }
}

I feel the first style is most readable and beautiful. In my opinion, it would be a bit inconvenient if we don't allow the style.

[jonas054]

I agree that the first style is very readable, but I don't think it's very common in the ruby world, so it can't be our only enforced style. Since this cop will support auto-correction I think we have three choices:

1. only allow the second and third style
2. allow all styles and auto-correct to left side alignment (which will be 2nd or 3rd style, depending)
3. make style a configuration option, allowing only one style (2nd/3rd by default), and auto-correct towards the configured style I prefer this one.

[bbatsov]

👍 for option 3. I think that the 3rd style should be the default one, since it's the most common. Our checks should obviously work for =>'s as well.

[jonas054]

My plan was to not really distinguish between 2nd and 3rd style, just leaving it as it is. Do you want auto-correction to change 2nd style into 3rd, @bbatsov? And do you want the configuration option to allow a choice between all three styles?

[bbatsov]

It'd be better if both styles are treated separated, so we could have proper auto-corrections for them. I'd suggest having a config option with 3 different values - the names of the 3 styles.

[jonas054]

Good point. I'll do it like that.

[yujinakayama]

👍 for option 3 too.