-
Notifications
You must be signed in to change notification settings - Fork 52
Enable cops in the Naming department #764
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
Conversation
106f40b to
ce85b4b
Compare
- [Naming/BlockParameterName](https://docs.rubocop.org/rubocop/cops_naming.html#namingblockparametername) - [Naming/ClassAndModuleCamelCase](https://docs.rubocop.org/rubocop/cops_naming.html#namingclassandmodulecamelcase) - [Naming/ConstantName](https://docs.rubocop.org/rubocop/cops_naming.html#namingconstantname) - [Naming/HeredocDelimiterCase](https://docs.rubocop.org/rubocop/cops_naming.html#namingheredocdelimitercase) - [Naming/MethodName](https://docs.rubocop.org/rubocop/cops_naming.html#namingmethodname) - [Naming/MethodParameterName](https://docs.rubocop.org/rubocop/cops_naming.html#namingmethodparametername) - [Naming/VariableName](https://docs.rubocop.org/rubocop/cops_naming.html#namingvariablename)
ce85b4b to
117885b
Compare
| VersionAdded: '0.53' | ||
| VersionChanged: '0.77' | ||
| MinNameLength: 3 | ||
| MinNameLength: 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the rationale for this? One would encourage names like x and y, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see how sometimes that might be needed though:
class Point
def initialize(x, y)
@x = x
@y = y
end
endThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this cop will little value with MinNameLength: 1. What if, instead, we add to AllowedNames?
AllowedNames:
# (merged with default list, including `id`, `db`, etc.)
- a
- b
- c
- x
- y
- zAnother option is for the cop to simply be disabled when it is a bad fit
Exclude:
- /path/to/point.rb
- /path/to/geometry/**There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not care about the size. What we are trying to get here is camelCase arguments that I found when enabling this cop.
sambostock
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple thoughts, but no blockers.
| AllowedPatterns: | ||
| - "\\Atest_" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove this? Don't we want to permit descriptive test names?
def test_rejects_HTTP_requests = ...
def test_accepts_HTTPS_requests = ...Not every project uses ActiveSupport::TestCase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is no reason to use acronyms in the test name. Just write everything downcased.
Uh oh!
There was an error while loading. Please reload this page.