Cop naming guidelines

[bbatsov]

With RuboCop 1.0 closer than ever it's time for us to start doing to final cleanup leading to the 1.0 release. One aspect of the cleanup is to make sure all cops follow consistent naming patterns (and we'll enforce those down the road). Here are a few examples of inconsistencies we have today:

• We use "Unneeded" and "Redundant" (we should use always "Redundant")
• Blank vs Empty (for lines)
• "Indent something" vs "something indentation"
• Mixing params and arguments here and there

I hope you get the idea. The goal of this ticket is to agree on how names should use in general, document our decisions, and update all cops to match those before 1.0.

We should do the same for cop configuration keys as well.

[bbatsov]

I was just given a great example of a bad cop name - Lint/PercentStringArray. Who can guess what this checks only based on its name? 😉

[maxh]

Thoughts:

(1) Disambiguate arguments and parameters. Spit cops if needed.

Arguments are in method invocations.
Parameters are in method definitions.

(2) For appropriate clustering in the documentation, I personally have found that clustering cops by their action (e.g. if they are powered by the same mixin) is better:

• IndentFoo
• IndentBar

Than having multiple similar cops spread out:

• FooIndentation
• BarIndentation

I was also going to suggest (3) Start cop names with verbs, such that cop names could be interpreted as "this cop is going to...", but that doesn't work for cops like SpaceInsideArrayLiteralBrackets which have multiple supported styles and therefore different verbs depending on the style. So for consistency with these cops, maybe "FooIndentation" is actually better for #2. (!)

[bbatsov]

@scottmatthewman Do you plan to continue working on this? I'd really like for us to clean up shop in the next couple of months, so we can do 1.0 around Christmas as well.

[scottmatthewman]

@bbatsov Yeah, I'd be happy to.

[scottmatthewman]

I've gone through all our cop names, and come up with some suggestions.

The biggest question is the "Indent something" vs "something indentation" question, which is pretty much limited to Layout cops.

In general, layout cop names describe their area of attention (their 'beat', if you wanted to take the police analogy further) and typically have multiple options for EnforcedStyle, along with zero or more additional elements.

I think the "NounAttentionType" works better for all these cops as a description of what they're looking for.

In terms of grouping, an alphabetical list of cops will therefore tend to cluster cops around language elements (arrays, hashes, etc.) rather than a particular type of attention. (I get that clustering by attention type is valid too – just that both naming styles produce rational clustering).

The good news is that there are fewer Align* and Indent*-style cops than there are *Alignment and *Indentation, so to switch from the former to the latter will be the least disruptive to our end users.

For non-layout cops, each cop's focus tends to be a lot tighter, and the cop names by and large reflect that well - however there are some that, if renamed, would provide additional consistency. I've got a suggested list for renaming below – although I have question marks against some:

• Layout/AlignArguments -> Layout/ArgumentAlignment
• Layout/AlignArray -> Layout/ArrayAlignment
• Layout/AlignHash -> Layout/HashAlignment
• Layout/AlignParameters -> Layout/ParameterAlignment
• Layout/IndentAssignment -> Layout/AssignmentIndentation
• Layout/IndentFirstArgument -> Layout/FirstArgumentIndentation
• Layout/IndentFirstArrayElement -> Layout/FirstArrayElementIndentation
• Layout/IndentFirstHashElement -> Layout/FirstHashElementIndentation
• Layout/IndentFirstParameter -> Layout/FirstParameterIndentation
• Layout/IndentHeredoc -> Layout/HeredocIndentation
• Layout/IndentationConsistency -> Layout/MethodIndentation see Note 1
• Layout/LeadingBlankLines -> Layout/LeadingEmptyLines
• Layout/TrailingBlankLines -> Layout/TrailingEmptyLines
• Lint/DuplicatedKey -> Lint/DuplicateKey
• Lint/HandleExceptions -> Lint/EmptyException
• Lint/MultipleCompare -> Lint/MultipleComparison
• Lint/PercentStringArray -> Lint/ExtraCharactersInPercentStringArray see Note 2
• Lint/PercentSymbolArray -> Lint/ExtraCharactersInPercentSymbolArray see Note 2
• Lint/StringConversionInInterpolation -> Lint/RedundantStringConversionInInterpolation
• Lint/UselessAccessModifier -> Lint/RedundantAccessModifier see Note 3
• Naming/UncommunicativeBlockParamName -> Naming/BlockParameterName
• Naming/UncommunicativeMethodParamName -> Naming/MethodParameterName
• Naming/VariableNumber -> Naming/VariableNumericSuffix
• Style/DefWithParentheses -> Style/MethodDefParenthesesWithNoArguments see note 4
• Style/PercentQLiterals -> Style/RedundantCapitalQ

Notes:

1. IndentationInconsistency is too vague a name, as inconsistency in one form or another is what all our layout cops are interested in!
2. I'm not sure that ExtraCharactersIn... is the right formation for these cops - can anyone come up with something pithier/more meaningful?
3. I'd recommend keeping the other Useless cops named as they are.
4. Yes, this is a long name – to be honest, it probably makes more sense in the long term to remove this cop and replace it with an option of Style/MethodDefParentheses

[bbatsov]

Great analysis! I agree with most of the suggestions, but I think there are a few nuanced names:

Layout/IndentationConsistency -> Layout/MethodIndentation see Note 1

I thought this would work on the top level as well.

Lint/DuplicatedKey -> Layout/DuplicateKey

Maybe DuplicateHashKey?

Lint/HandleExceptions -> Lint/EmptyException

I think SuppressedException would be a better name here or something along those lines.

Lint/PercentStringArray -> Lint/ExtraCharactersInPercentStringArray see Note 2

That's tricky indeed. I think it should be something along the lines of RedundantQuotes..., but I'm not 100% about this.

Lint/StringConversionInInterpolation -> Lint/RedundantStringConversionInInterpolation

Maybe Coercion instead of Conversion?

Lint/UselessAccessModifier -> Lint/RedundantAccessModifier see Note 3

I think here the point is to highlight that the modifier doesn't do what people expect that it does, so I'm not sure about this name.

Naming/VariableNumber -> Naming/VariableNumericSuffix

There's also the point if this pertains to variables or to all identifiers.

Yes, this is a long name – to be honest, it probably makes more sense in the long term to remove this cop and replace it with an option of Style/MethodDefParentheses

Totally agree. I think we had a plant to do this at some point, but maybe the implementation was going to be too complex or something like this.

I think that you can go and rename the clear-cut cases and we can discuss additionally the rest. It would also be nice to add some naming practices in the docs and some internal cop that checks for cop classes that use "forbidden" words and points to the docs.

[scottmatthewman]

While I initially thought that Style/PercentQLiterals ought to join the newly-renamed Style/RedundantCapitalW as Style/RedundantCapitalQ, in reality it supports a configurable style that allows use of %Q in all cases. (docs)

In this way, Style/PercentQLiterals is more closely aligned with Style/StringLiterals, which similarly allows for a configuration enforcing the use of double quotes in all places.

For this reason, I think the current name is actually more consistent. Indeed, I think that we'd have better consistency if we replaced Style/RedundantCapitalW with a cop that has similar configuration options to the %Q and string literals cops.

[pirj]

To sum this up, what's left:

1. Layout/IndentationConsistency -> Layout/MethodIndentation Layout/InconsistentIndentation.

2. Lint/PercentStringArray: This cop checks for quotes and commas in %w, e.g. %w('foo', "bar")
   I'd go with Lint/RedundantPercentWPunctuation, or Lint/ExtraPercentWPunctuation.

3. Lint/PercentSymbolArray: This cop checks for colons and commas in %i, e.g. %i(:foo, :bar)
Lint/RedundantPercentIPunctuation or Lint/ExtraPercentIPunctuation?

4. Lint/UselessAccessModifier: This cop checks for redundant access modifiers, including those with no code, those which are repeated, and leading public modifiers in a class or module body.
   Let's keep the name. In some cases, e.g. duplication, it's redundant, but in the others - it's useless.

5. Naming/VariableNumber: This cop makes sure that all numbered variables use the configured style, snake_case, normal case, or non_integer, for their numbering.

Naming/VariableNumber -> Naming/VariableNumericSuffix

There's also the point if this pertains to variables or to all identifiers.

It's only for variables (on_arg, on_lvasgn, on_ivasgn, on_cvasgn).

I'd go with Naming/NumericVariableSuffix as it reads better to me.

6. Style/DefWithParentheses: This cop checks for parentheses in the definition of a method, that does not take any arguments. Both instance and class/singleton methods are checked.

Style/RedundantMethodDefParentheses?

7. Style/PercentQLiterals

%q(...) behaves like a single-quote string (no interpolation or character escaping), while %Q behaves as a double-quote string.

My voice goes for no naming change. There's a Style/StringLiterals which does the same for quotes.

TODO (please feel free to edit):

• Layout/IndentationConsistency -> Layout/InconsistentIndentation
• Lint/PercentStringArray -> Lint/RedundantPercentWPunctuation
• Lint/PercentSymbolArray -> Lint/RedundantPercentIPunctuation
• Naming/VariableNumber -> Naming/NumericVariableSuffix
• Style/DefWithParentheses -> Style/RedundantMethodDefParentheses

Regarding the naming guideline itself, does the following sound good enough?

Name the cop to explain the offense it detects, not where or what the subject of the offense is.

[pirj]

@scottmatthewman Would you like to cover this last mile yourself, or do you need a hand with that?

[bbatsov]

We also need to put something in the docs on the subject and maybe introduce an internal affairs cop that checks for some naming anti-patterns.

[scottmatthewman]

@scottmatthewman Would you like to cover this last mile yourself, or do you need a hand with that?

I'm afraid I'll have to pass at the moment, due to other pressures