ManageIQ/guides

Can we use community styles for Percent Array literals?

NickLaMuro opened this issue · 19 comments

NickLaMuro commented

I have been frustrated with our current override of the defaults for a while now, and have not been completely silent about it in the past:

ManageIQ/manageiq-providers-amazon#434 (comment)
ManageIQ/manageiq-providers-amazon#434 (comment)

And I don't think I am alone...

I would like to petition we change this back to the community styles.

(I also willingly take on the work to update our code bases to use this style, as this would very much ease a long standing "erk-some" I have had with it)

Reference links

Fryguy commented

The only reason we had the overrides in the first place was because a) those were the original rubocop styles at the beginning until they changed it and b) our entire codebase was already that way.

Since there was never any good reason for it I am 👍 with going to the newer rubocop defaults.

(I also willingly take on the work to update our code bases to use this style, as this would very much ease a long standing "erk-some" I have had with it)

Isn't it more or less just a rubocop -a command?

NickLaMuro commented

Isn't it more or less just a rubocop -a command?

Yeah, but I assume it is over multiple repos and with plenty of edge cases. My main point is that I wasn't trying to off load work to some one else for a personal gripe that I had.

  • 👍2
djberg96 commented

👍

bdunne commented

I opened #286 to make rubocop properly enforce the cop that we had set in the config file.

My opinion: I don't like to use the %w type shortcuts at all. I find ["a", "b"] much easier than trying to remember whether %w gives me strings or symbols in it's effort to save a few characters.

NickLaMuro commented

I opened #286 to make rubocop properly enforce the cop that we had set in the config file.

@bdunne Huh, I thought you were just putting us back to our previous state after rubocop got bumped to make our code the same as it was. TIL

My opinion: I don't like to use the %w type shortcuts at all...

I actually don't mind this opinion either (and frequently forget to use them myself). Unfortunately, with how things are configured currently, you can't do this with out being dinged by the bot because of these rules which we have enabled (defaults?):

If we wanted to invert these rules, or even remove them entirely, I would be fine doing that as well. If we were to remove the requirement for requiring the use of "percent arrays", I would still suggest we switch our customization.

But as it stands now, I am force to use %w() whenever I have a word array, which is pretty common and very frustrating.

Fryguy commented

yeah, I wasn't speaking about #286 specifically, but the decision before that that set default: ()

Fryguy commented

My opinion: I don't like to use the %w type shortcuts at all. I find ["a", "b"] much easier than trying to remember whether %w gives me strings or symbols in it's effort to save a few characters.

I noticed that WordArray and SymbolArray both have a "minimum size". What do you think of that @bdunne ? I agree with @NickLaMuro that either way, we should use the "defaults"

bdunne commented

I noticed that WordArray and SymbolArray both have a "minimum size".

I think that only makes things more confusing. I think we should pick either Array of strings/symbols or use the shortcuts, not some mix dependent on the length.

Alternatively, we could disable the cop altogether.

NickLaMuro commented

Alternatively, we could disable the cop altogether.

To clarify: remove WordArray and SymbolArray cops?

Again, I would be fine with that, as mentioned in my previous comment, which would effectively mean we don't mind what array type you use. Personally, that really doesn't bother me to not enforce array type, but I could see it being a slippery slop to say certain cops shouldn't be enforced (leading to "Should we just disable rubocop all together?" PRs). For what it is worth, there are definitely cases of both within the codebase still, so we are not compliant with forcing one or the other either way.

Again, if we do decide to disable WordArray and SymbolArray, I would still argue for the original intent of this PR of switching back to the defaults for PercentLiterals as well.

Fryguy commented

I agree @NickLaMuro . I think we should disable WordArray and SymbolArray (so we don't care on which style the user uses). However, when they decide to use %w or %i , the preferred delimiter is the default.

  • 👍1
Fryguy commented

@NickLaMuro Now that we have a handful of choices, perhaps you can open up a talk article pointing people to this issue, and allowing them to vote via reaction emoji (and then give it a week or so - maybe a little more since some people may get weekly digests or something)

  • 👍1
NickLaMuro commented

@Fryguy Woops read your first comment and then decided to open a starter PR: #334

Will mark it as [WIP] as we vote in the forum, but it gets things started with the easy change (relaxing the array style enforcement to allow either normal or symbol/word arrays). Going to create a talk article now and reference to this.

  • ❤️1
NickLaMuro commented

Talk article created:

http://talk.manageiq.org/t/vote-ruby-rubocop-array-styles/4060

Let's move most of our discussion over to there from now on.

himdel commented

Isn't it missing a fifth option, where all of those are good?

I won't vote for anything that makes %w() bad, I'm too used to it. (And it's closer to the perl default too.)
But I'm not OCD enough to force people to not use %w[] if they're used to it from elsewhere.
(I don't care about arrays at all, %w(foo bar) is IMO more readable and easier to write than the whole punctuation thing but OTOH there are probably valid use cases where you want to quote everything.)

  • 👍1
NickLaMuro commented

Isn't it missing a fifth option, where all of those are good?

@himdel are you saying where we don't enforce any particular Percent literal style as well? Not sure I agree with that, since I haven't spent much time with perl myself. But also, while maybe perl inspired, I don't know that perl allows you to change the delimiter to whatever you feel like:

# all valid...

%w(foo bar baz)
%w[foo bar baz]
%w{foo bar baz}
%w|foo bar baz|
%w%foo bar baz%  # why would you even...
%w!foo bar baz!  # ò_ô
%w"foo bar baz"  # (╯°□°)╯︵ sʎɐɹɹɐ pɹoʍ

#=> ["foo", "bar", "baz"]

So I think allowing "anything goes" is just... well, not a good idea...

Doubt rubocop is granular enough to allow N different types of delimiters as well, though I guess I haven't looked into it.

himdel commented

OK, I get that :) In that case, voted for 2, sorry :).

Perl does allow you to use any kind of paired unicode char actually, but qw() was always the default.

NickLaMuro commented

At least it isn't so bad to allow %wwfoo bar bazw...

OK, I get that :) In that case, voted for 2, sorry :).

No worries, it is honestly one I could live with if required to (though, currently, the votes aren't in your favor 😉 )

himdel commented

currently, the votes aren't in your favor ;)

That, I can live with :)

Fryguy commented

Closed via #334