Updates to rubocop_rails / rubocop linters #19
Conversation
- Exclude the `bin` and `config` directories. - Disable `Style/BlockComments`: I see no reason to not allow block comments, and they get used in third-party documentation (spec_helper). - Ignore `seeds.rb` for block length, line length, and module length. - Ignore single/double quotes in generated files (Gemfile, Rakefile, etc)
There was a problem hiding this comment.
Overall, I think this may benefit from smaller, more targeted changes so they can be discussed specifically, as this is addressing a number of (as far as I can tell) unrelated things, some of which the team may agree to and others the team may not. I'm not sure if combining them together makes sense.
However, given they're all here now, I've provided my comments/thoughts on each. If this gets broken out into separate PRs, please consider these comments in those subsequent PRs as part of the discussion.
| @@ -41,3 +41,6 @@ Metrics/BlockLength: | |||
| Metrics/ModuleLength: | |||
| Exclude: | |||
| - 'spec/**/*' | |||
|
|
|||
| Style/BlockComments: | |||
| Enabled: false | |||
There was a problem hiding this comment.
Based on your description comment, I'm not sure I follow where you see block comments in a spec helper (by default) that you want to retain. From the documentation describing block comments:
This cop looks for uses of block comments (=begin…=end).
# bad
=begin
Multiple lines
of comments...
=end
# good
# Multiple lines
# of comments...
Moreover, while consistency throughout an application is important, a primary concern of this style configuration is for managing code that we write, not necessarily those of our dependencies (however, I understand the confusion/tension that generated/default code that doesn't adhere to the standard can create).
I personally don't see a need to introduce block comments, and am fine with the default of warning against them.
|
|
||
| Metrics/LineLength: | ||
| Exclude: | ||
| - 'db/seeds.rb' |
There was a problem hiding this comment.
Why shouldn't we adhere to our line length convention in this file?
| Style/StringLiterals: | ||
| Exclude: | ||
| - 'config.ru' | ||
| - 'Gemfile' |
There was a problem hiding this comment.
The Gemfile is excluded from the double-quotes cop in our base cop already.
| Metrics/BlockLength: | ||
| Exclude: | ||
| - 'db/seeds.rb' | ||
| - 'spec/**/*' |
There was a problem hiding this comment.
This is already disabled in our base cop file. This should be unnecessary, and if you're seeing notifications for violating this cop in spec files right now, that points to a different bug or configuration setting that should be addressed differently from specifying it here.
| - 'vendor/**/*' | ||
|
|
||
| Rails: | ||
| Enabled: true | ||
|
|
||
| Metrics/BlockLength: | ||
| Exclude: | ||
| - 'db/seeds.rb' |
There was a problem hiding this comment.
What's the justification for removing block length notifications in seeds? I see no problem with being warned about a long block in this location.
| - 'config.ru' | ||
| - 'Gemfile' | ||
| - 'Rakefile' | ||
| - 'spec/rails_helper.rb' |
There was a problem hiding this comment.
This file in particular is likely to have things changed in it over time, and I think it should adhere to our quoting standard.
| @@ -3,11 +3,34 @@ inherit_from: rubocop.yml | |||
| AllCops: | |||
| Exclude: | |||
| - db/schema.rb | |||
| - 'bin/**/*' | |||
| @@ -3,11 +3,34 @@ inherit_from: rubocop.yml | |||
| AllCops: | |||
| Exclude: | |||
| - db/schema.rb | |||
| - 'bin/**/*' | |||
| - 'config/**/*' | |||
There was a problem hiding this comment.
If we change things in here, I'd rather see that they adhere to our coding standard; however, I know that some cops are violated out of the box, and also that some, like module length, may not make sense to adhere to in these files. However, turning off all cops seems a bit too generic for me.
| Exclude: | ||
| - 'config.ru' | ||
| - 'Gemfile' | ||
| - 'Rakefile' |
There was a problem hiding this comment.
Specific for config.ru and Rakefile, is the concern just to get a clean rubocop run with no warnings with generated code? If so, the autoformatter could clean that up for you quickly, and specifically for those 2 files they're unlikely to churn/be seen much over the life of the project.
| - 'db/seeds.rb' | ||
| - 'spec/**/*' | ||
|
|
||
| Style/StringLiterals: |
There was a problem hiding this comment.
Note that there is an open issue in gnarails so that projects generated via that template have the provided code adhere to our coding standards defined here out of the box.
|
@taylorkearns how can I help with this? |
|
Sorry to lag on this, it seems like perhaps this PR should be closed and we should open up smaller PRs to more completely discuss/merge specific changes. Does that sound accurate? |
|
That works for me. |
binandconfigdirectories.Style/BlockComments: I see no reason to not allow block comments, and they get used in third-party documentation (spec_helper).seeds.rbfor block length, line length, and module length.