Implement some parameter name check cop

[bbatsov]

People keep referring to Reek's Uncommunicative Variable Name check as one of the big omissions in RuboCop. We didn't include it initially, as it can't be implemented reliably (some short names are pretty meaningful - like x and y when dealing with points), but I guess if the users want it we should give it to them.

This cop should act on both method and block param names.

[palkan]

@bbatsov

Are we going to use the same checks as Reek does?
More precisely:

• 1-letter names
• capital letters
• names ending with a number.

I'm not sure about "names beginning with an underscore" because it's a common pattern for naming unused vars when overriding methods (kinda Erlang style).

And what about this hash.each_key { |k| p k }?
Block params are very likely to be one-letters. I guess, we should check them only for other offenses (capitals, numbers).

And I think, we should have a flexible configuration here, smth like:

Style/UncommunicativeName:
  # Do not report offenses on the provided names
  IgnoreNames: []

[bbatsov]

I'm not sure about "names beginning with an underscore" because it's a common pattern for naming unused vars when overriding methods (kinda Erlang style).

I wouldn't check about this.

And what about this hash.each_key { |k| p k }?
Block params are very likely to be one-letters. I guess, we should check them only for other offenses (capitals, numbers).

Might make sense to have 2 cops for method params and block param names. At any rate the cop should have a rather flexible configuration.

[palkan]

Might make sense to have 2 cops for method params and block param names. At any rate the cop should have a rather flexible configuration.

And what about variable names? Yet another cop?

[garettarrowood]

I've been thinking about this and would like to tackle it. Here is my proposal:

This issue can represent the addition of 2 Cops.

1. Style/UncommunicativeBlockParamName
2. Style/UncommunicativeMethodArgName

After those, we can discuss the possibility of other reek Uncommunicative Names. Although, at first inspection they look like they are possibly overkill.

To accommodate meaningful single char names, configuration could be added to whitelist particular letters. OR we can just allow them all for block params & never for method. I'd love to go as far as including a little strong typing here, by checking that the class of a single char var match a value passed to configuration. But we can't and shouldn't do that in RuboCop.

Is there any appropriate place in Rubocop to allow code owners to specify what Class those single chars should represent but not enforce it. e.x. { h: [Hash], i: [Integer, Numeric], c: [CustomClass], etc. }? I think this is just wishful thinking on my part, but wanted to bring up the possibility.

@bbatsov @palkan - Any last thoughts on this?

[palkan]

OR we can just allow them all for block params & never for method

I prefer this way. It's very common to have single char (or less meaningful) block params names, especially for keys/values (|k|, |k, v|).

Methods are likely to have (and, IMO, should have) more descriptive arguments names though.

Is there any appropriate place in Rubocop to allow code owners to specify what Class those single chars should represent but not enforce it

It can be configurable in Rubocop, but I think in most cases there gonna be the whole alphabet) Using the first char of a class name is popular.

Of course, having good list of defaults (k, v, h, i, j, t, for example) and disable this cop by default still make sense.

[garettarrowood]

So to recap:

Style/UncommunicativeBlockParamName -

1. No capital letters
2. No names ending in numbers
3. Config that allows one to whitelist param names that break these rules.

Style/UncommunicativeMethodArgName -

1. No single letter names
2. No capital letters
3. No names ending in numbers
4. Config that allows one to whitelist arg names that break these rules.

And disable this cop in the rubocop code base by default.

[palkan]

Style/UncommunicativeBlockParamName -

1. No capital letters
2. No names ending in numbers
3. Config that allows one to whitelist param names that break these rules.

And I think:

4. Predefined list of accepted names (such as k, v, etc); not sure that that should be a default whitelist, maybe, a separate list (always included).

And according to:

No single letter names

What about having a parameter MinArgNameLength to make it even more configurable. And, I think, default value of 3 is better (2-letter names? not sure).

[garettarrowood]

Ok. Ready to start crafting this over the next few days. I'm still not sold on passing a predefined list of exceptions to the rule. But I really like the idea of a MinArgNameLength and think something equivalent would work well for the blocks as a MinParamNameLength. MinArg could default to 3, but MinParam could default to 1, making it more up to the users to actively enforce block param length restrictions.

In this first pass, I'm going to forego "Config that allows one to whitelist param names that break these rules." Since this is addition and not a replacement of existing functionality, let's get something that makes sense up and working. And disable it by default. Then we can then raise additional issues to add more configuration, whatever that may be.