Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Renamed Naming/VariableNumber cop to Naming/IdentifierNumber. Extended this cop to handle symbols and method names #8964

Merged
merged 1 commit into from
Nov 3, 2020

Conversation

fatkodima
Copy link
Contributor

This PR renames and extends an existing cop to also handle symbols and method names, as in the style guide https://rubystyle.guide/#snake-case-symbols-methods-vars-with-numbers

@koic
Copy link
Member

koic commented Oct 29, 2020

RuboCop has reached 1.0, so we will need to consider providing it to prevent breaking changes.
@rubocop-hq/rubocop-core

@@ -31,6 +31,7 @@ class ConfigObsoletion
'Lint/UnneededSplatExpansion' => 'Lint/RedundantSplatExpansion',
'Naming/UncommunicativeBlockParamName' => 'Naming/BlockParameterName',
'Naming/UncommunicativeMethodParamName' => 'Naming/MethodParameterName',
'Naming/VariableNumber' => 'Naming/IdentifierNumber',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO, 1.0 stable may require a (new) mechanism that can provide a migration period to users for deprecated cop name until 2.0 is reached.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I totally agree. It'd be nice to have a mechanism which allows us to change the name of a cop without breaking configuration. E.g. cops can have a primary name and a deprecated name or something like this.

@bbatsov
Copy link
Collaborator

bbatsov commented Oct 29, 2020

Yeah, definitely we won't be renaming any cops unless we introduce a way to do so without breaking the user configs.

@bbatsov
Copy link
Collaborator

bbatsov commented Nov 2, 2020

@fatkodima I've been thinking we can make the changes without renaming the cop, as probably the impact for the identifiers will be relatively small. We can mitigate it further with boolean configuration flags for each identifier type.

@fatkodima
Copy link
Contributor Author

Yes, as an option. Updated with your suggestion.

@bbatsov
Copy link
Collaborator

bbatsov commented Nov 3, 2020

Method params were represented as locals in the AST and therefore are covered as well even before your changes, right? It'd be nice to reflect this in the cop examples.

@fatkodima
Copy link
Contributor Author

Added examples with arguments to the docs.

@bbatsov bbatsov merged commit 51ff1d7 into rubocop:master Nov 3, 2020
@bbatsov
Copy link
Collaborator

bbatsov commented Nov 3, 2020

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants