[#2495, #1609] Add EnforcedStyle config parameter to IndentArray

[jawshooah]

For parity with IndentHash, SupportedStyles include:

• special_inside_parentheses(default)
• consistent
• align_brackets

[alexdowad]

Awesome!! Thanks!! 👍

[jawshooah]

RuboCop's own code has quite a few offenses, due to following the consistent style (which was de-facto enforced before this PR). Should we:

1. set EnforcedStyle to consistent in config/default.yml,
2. set EnforcedStyle to consistent in .rubocop.yml, or
3. fix the offenses?

[jawshooah]

There's also a huge amount of duplication between IndentArray and IndentHash, since I basically just copy-pasted, removed logic that was applicable to hashes but not arrays, and tweaked some identifier names.

Is it worth extracting the common logic into a shared module?

[alexdowad]

RuboCop's own code has quite a few offenses

I say it's better to fix all the offenses. The cleaner and more consistent the codebase is, the better.

[alexdowad]

Is it worth extracting the common logic into a shared module?

Don't know what @bbatsov wants, but I would say, follow the "three strikes" rule. If the same code is needed in a 3rd place, then extract it.

It may be that there are other places in the codebase which could benefit from the extracted code right now... if so, it would be better to unify them.

[alexdowad]

I say it's better to fix all the offenses.

...Wait a minute. Sorry.

Do we really want special_inside_parentheses to be the default style for IndentArray?
Is this the style which most Ruby developers will prefer?

[jawshooah]

Is this the style which most Ruby developers will prefer?

Not sure. It could certainly make it a pain to keep lines within a reasonable length, although in most cases you'd just have to change something like this:

oh.wow.this.is.a.really.long.chain.of.methods([
                                                a.really.really.long.value,
                                                another.really.really.long.value
                                              ])

to this:

oh.wow.this.is.a.really.long.chain.of.methods(
  [
    a.really.really.long.value,
    another.really.really.long.value
  ])

In any case, I've pushed fixes for the various offenses. Check them out in e4f17a2, and if the changes look unreasonable, we can just switch the default back to consistent.

[alexdowad]

Hmm. In some cases, the new style looks really good. In others, not so much.

[jawshooah]

Yup. Unfortunately I'm not sure that any hard requirement will look good in all scenarios.

consistent has been the de-facto default for a while now, so maybe we should stick with that. Might cause confusion since the default for IndentHash is special_inside_parentheses, though.

[jawshooah]

I'll ping @jfelchner and @zenspider here as well, since they opened the issues we aim to resolve.

[alexdowad]

On the balance, I think special_inside_parentheses looks a little better...

[jfelchner]

@jawshooah thanks so much! Defaults don't matter to me as long as I can switch it. 😉

[jawshooah]

@bbatsov, care to weigh in on what the default should be?

[jfelchner]

@jawshooah what does the current impl do? It should probably match that.

[jawshooah]

@jfelchner I'd be fine with that, but as I said above:

consistent has been the de-facto default for a while now, so maybe we should stick with that. Might cause confusion since the default for IndentHash is special_inside_parentheses, though.

[jfelchner]

@jawshooah ah thanks for pointing that out. I guess this is definitely a question for @bbatsov 😊

[bbatsov]

On the balance, I think special_inside_parentheses looks a little better...

Yeah (plus it's how Emacs's ruby-mode works :-) ).

A changelog entry has to be added and the PR has to be rebased.

[jawshooah]

@bbatsov I've addressed your comments and rebased.

[bbatsov]

👍 Thanks!