Vote! Ruby rubocop array styles


#1

Myself and others over the years have had some disagreements with how rubocop is currently configured for ManageIQ, specifically with array styles. The main gripe being our deviation from the default Style rules (Cops) for Percent literals (we use %w() for word arrays instead of %w[] ), but others also don’t like the short hand and would prefer to use normal array syntax.

I have opened up a issue on github where we have had some initial discussion and decided to leave this to a vote.

The options are:

  1. Relax enforcement of Word/Symbol Arrays, and change Percent Literals to rubocop defaults

    # word arrays
    ["foo", "bar", "baz"] # good
    %w[foo bar baz]       # good
    %w(foo bar baz)       # bad
    
    # symbol arrays
    [:foo, :bar, :baz]    # good
    %i[foo bar baz]       # good
    %i(foo bar baz)       # bad
    
  2. Only relax the enforcement on Word/Symbol Arrays

    # word arrays
    ["foo", "bar", "baz"] # good
    %w[foo bar baz]       # bad
    %w(foo bar baz)       # good
    
    # symbol arrays
    [:foo, :bar, :baz]    # good
    %i[foo bar baz]       # bad
    %i(foo bar baz)       # good
    
  3. Only revert our changes to Percent literal styles

    # word arrays
    ["foo", "bar", "baz"] # bad
    %w[foo bar baz]       # good
    %w(foo bar baz)       # bad
    
    # symbol arrays
    [:foo, :bar, :baz]    # bad
    %i[foo bar baz]       # good
    %i(foo bar baz)       # bad
    
  4. Leave everything as is

    # word arrays
    ["foo", "bar", "baz"] # bad
    %w[foo bar baz]       # bad
    %w(foo bar baz)       # good
    
    # symbol arrays
    [:foo, :bar, :baz]    # bad
    %i[foo bar baz]       # bad
    %i(foo bar baz)       # good
    

Additional documentation for these rules can be found in the links below:

https://www.rubydoc.info/gems/rubocop/0.52.1/RuboCop/Cop/Style/PercentLiteralDelimiters
https://www.rubydoc.info/gems/rubocop/0.52.1/RuboCop/Cop/Style/SymbolArray
https://www.rubydoc.info/gems/rubocop/0.52.1/RuboCop/Cop/Style/WordArray

Assuming I can build a poll for this thing, please cast your vote and let us know what you think!

We don’t have a set timeline for implementing this change, but we probably are going to give this a couple of weeks before moving forward with a change.

Thanks!
-Nick

  • Both: Relax array enforcement and use Percent Literal defaults
  • Only relax the enforcement on Word/Symbol Arrays
  • Only revert our changes to Percent Literal styles
  • Don’t change anything! (you monsters…)

0 voters


#2

Can you add options to the poll for good/good/good (all 3 cops off) and bad/good/good (PercentLiteralDelimiters off, SymbolArray & WordArray enabled)?

I strongly want the right to write %w[] and %i[] in new code, because well these are arrays, but I’m unsure it’s worth outlawing existing %w() / %i() code (don’t feel it justifies the noise of a mass-reformat commit, and don’t want warnings for old code either; but to be clear, I won’t actively oppose a mass reformat).

I mildly prefer human discretion on when ["foo"] reads better than %w[foo] in context, so personally I’d prefer the SymbolArray & WordArray cops off, or at least set something like MinSize: 3


#3

For me:

["foo", "bar", "baz"] # bad
%w[foo bar baz]       # good
%w(foo bar baz)       # good

We have the %w(foo bar baz) form in too many places and people are used to it. I do not think it’s worth changing all those places.

I uderstand that the new best way is %w[foo bar baz] and have no problem using that.


#4

Replying to both @cben and @martinpovolny here, since they asked similar questions:

Can you add options to the poll for good/good/good (all 3 cops off) and bad/good/good (PercentLiteralDelimiters off, SymbolArray & WordArray enabled)?

and…

For me:

["foo", "bar", "baz"] # bad
%w[foo bar baz]       # good
%w(foo bar baz)       # good

We have the %w(foo bar baz) form in too many places and people are used to it. I do not think it’s worth changing all those places.

I have actually elaborated on this point in the original issue on Github (links to a specific comment):

But effectively, I think being too lax is a bad idea, because it:

a) starts down a slippery slop of “does rubocop even serve a purpose at this point?”
b) Specifically with percent literals, any non-word/non-number character goes (see link for some examples)

So I think we should have an enforcement one way or the other on PercentLiterals, and I would prefer the bracketed form myself (but the vote will ultimately decide).

Regarding “outlawing existing code”, we already handle this in most cases repos since the bot (@miq_bot) is git diff aware, and won’t punish a PR for invalid rules existing somewhere else in the code base. We in fact already have places in ManageIQ/manageiq even where this is currently breaking the style:



Beyond that, I plan on volunteering to do a rubocop -a on all ManageIQ/* repos (which does the find and replace automatically) if the switch goes through, so you don’t have to worry your “pretty little faces” about having to deal with that. I felt it fair that if I am going to go to great lengths to complain about it, I would be the one to go around fixing it as well. :smile:

Hope that covers things! If not, yell in the comments some more and mob mentality might force me to add the extra poll options… :worried: