To discuss: relaxed "case-by-case consistency" setting for layout cops

[zverok]

Sorry in advance for the long and informal ticket, and if it is not the right time or place to post it.

I'd like to discuss an idea of the "case-by-case" consistency of layout cops (and probably some other cops of "personal preference" class). I'll show an idea on only one example of Layout/AlignParameters cop, but I believe it is relevant for a lot of cases.

So, here is what style guide says (abbreviated):

# bad (double indent)
Mailer.deliver(
    to: 'bob@example.com',
    from: 'us@example.com',

# good
Mailer.deliver(to: 'bob@example.com',
               from: 'us@example.com',
               subject: 'Important message',

# good (normal indent)
Mailer.deliver(
  to: 'bob@example.com',
  from: 'us@example.com',

So, style guide's recommendation, basically: "don't do unexplainable indents". Now, if we look at the relevant cop's settings...

Layout/ArgumentAlignment:
  Description: Align the arguments of a method call if they span more than one line.
  ...skip...
  EnforcedStyle: with_first_argument
  SupportedStyles:
  - with_first_argument         <<<<
  - with_fixed_indentation      <<<<

There is no way to ask the cop "just check that each call doesn't have unexplainable indentation, either of two styles is OK".

Why is this bad?

1. As "just a matter of personal preference/how it looks" (which existence of the setting clearly shows), the current set of settings makes a hard pressure on "always do the same thing, even if it looks bad". For one, I mostly prefer with_first_argument, except some cases like this:

has_and_belongs_to_many :users
                        :aint, this: :one,
                        a_bit: -> { too_far? }

TBH, through last year, we switched a large codebase from with_first_argument to with_fixed_indentation and back twice, depending on what part of the system we closely looked at when discussed "which looks better/more readable". With one of the stated goals of Rubocop to "avoid bikeshedding", for our team absence of an option "both are OK, just do the sane thing" led to exactly opposite direction :)
2. When introducing Rubocop (or turning on previously disabled cops with codebase maturing), it may happen that some modules were written in one style, and another in another one. Probably it can be fixed with "executive decision"™, but in a lot of cases, when after a brief experiment people see that sticking to either option leads to 100+ files PR (and requires probably some manual tweaking of weirdly shifted comments and such stuff), they frequently choose to do neither, disabling the useful cop.

So, the point is, a lot of cops have "this way or that way" setting, but their main goal is to avoid the unstated third option: "no consistent alignment/spacing", and it would be good to make this option explicit: "do whatever you want, but keep it clear and readable".

One problem with this option would be "what auto-correct should do"? Imagining this code:

File.write('out.yml',
       some.data.to_yml)

...it is hard (and probably unnecessary) to guess what is the best auto-correct -- whether it was previously shorter method name (so first and second argument were aligned), or it was some foo = .... (and the second line was two-spaces shifted). I believe in this case it would be OK to do nothing, or just fix cases like this:

some_method(foo,
   bar,
     baz,
  blah)

...to some common indentation, even if it is just to the first carry-over argument's:

some_method(foo,
   bar,
   baz,
   blah)

Rubocop will be still (rightfully) complaining, but now it would be much easier to fix (just shift everything to the desired column) than before.

Sorry, again, for the long text!

[bbatsov]

I agree with your suggestion - it certainly makes sense to me. As you pointed out yourself the main complexity of having this comes from the fact that we'll still need to default to something in case of ambiguity. I seem to recall we have a few cops supporting similar behaviour, but no concrete examples come to mind.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution and understanding!

[zverok]

Just in case I'll forget: I plan to do some experimenting in January on those matters.
My current idea is that the best (easy to implement and least disruptive) option is to have AllowedStyles generic param in many cops, so it would be one enforced and many allowed; with any of "allowed" considered OK (not changed to enforced), but if something matches neither, it would be converted to "enforced" (as usual).