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
All 11 comments
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? 😉
bbatsov
on 23 May 2019
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. (!)
maxh
on 23 May 2019
@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.
bbatsov
on 25 Oct 2019
@bbatsov Yeah, I'd be happy to.
scottmatthewman
on 25 Oct 2019
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/ArgumentAlignmentLayout/AlignArray->Layout/ArrayAlignmentLayout/AlignHash->Layout/HashAlignmentLayout/AlignParameters->Layout/ParameterAlignmentLayout/IndentAssignment->Layout/AssignmentIndentationLayout/IndentFirstArgument->Layout/FirstArgumentIndentationLayout/IndentFirstArrayElement->Layout/FirstArrayElementIndentationLayout/IndentFirstHashElement->Layout/FirstHashElementIndentationLayout/IndentFirstParameter->Layout/FirstParameterIndentationLayout/IndentHeredoc->Layout/HeredocIndentationLayout/IndentationConsistency->Layout/MethodIndentation_see Note 1_Layout/LeadingBlankLines->Layout/LeadingEmptyLinesLayout/TrailingBlankLines->Layout/TrailingEmptyLinesLint/DuplicatedKey->Lint/DuplicateKeyLint/HandleExceptions->Lint/EmptyExceptionLint/MultipleCompare->Lint/MultipleComparisonLint/PercentStringArray->Lint/ExtraCharactersInPercentStringArray_see Note 2_Lint/PercentSymbolArray->Lint/ExtraCharactersInPercentSymbolArray_see Note 2_Lint/StringConversionInInterpolation->Lint/RedundantStringConversionInInterpolationLint/UselessAccessModifier->Lint/RedundantAccessModifier_see Note 3_Naming/UncommunicativeBlockParamName->Naming/BlockParameterNameNaming/UncommunicativeMethodParamName->Naming/MethodParameterNameNaming/VariableNumber->Naming/VariableNumericSuffixStyle/DefWithParentheses->Style/MethodDefParenthesesWithNoArguments_see note 4_Style/PercentQLiterals->Style/RedundantCapitalQ
Notes:
IndentationInconsistencyis too vague a name, as inconsistency in one form or another is what all our layout cops are interested in!- I'm not sure that
ExtraCharactersIn...is the right formation for these cops - can anyone come up with something pithier/more meaningful? - I'd recommend keeping the other
Uselesscops named as they are. - 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
scottmatthewman
on 25 Oct 2019
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.
bbatsov
on 26 Oct 2019
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.
scottmatthewman
on 29 Oct 2019
To sum this up, what's left:
Layout/IndentationConsistency->Layout/MethodIndentation.Lint/PercentStringArray: This cop checks for quotes and commas in %w, e.g.%w('foo', "bar")
I'd go withLint/RedundantPercentWPunctuation, orLint/ExtraPercentWPunctuation.Lint/PercentSymbolArray: This cop checks for colons and commas in %i, e.g.%i(:foo, :bar)
Lint/RedundantPercentIPunctuationorLint/ExtraPercentIPunctuation?Lint/UselessAccessModifier: This cop checks for redundant access modifiers, including those with no code, those which are repeated, and leadingpublicmodifiers 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.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.
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?
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/MethodIndentation - [ ]
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
on 9 Aug 2020
@scottmatthewman Would you like to cover this last mile yourself, or do you need a hand with that?
pirj
on 19 Aug 2020
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.
bbatsov
on 19 Aug 2020
@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
scottmatthewman
on 19 Aug 2020
Related issues
kirrmann
·
3Comments
herwinw
·
3Comments
millisami
·
3Comments
bquorning
·
3Comments
Ana06
·
3Comments
Most helpful comment
Great analysis! I agree with most of the suggestions, but I think there are a few nuanced names:
I thought this would work on the top level as well.
Maybe
DuplicateHashKey?I think
SuppressedExceptionwould be a better name here or something along those lines.That's tricky indeed. I think it should be something along the lines of
RedundantQuotes..., but I'm not 100% about this.Maybe
Coercioninstead ofConversion?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.
There's also the point if this pertains to variables or to all identifiers.
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.