Rubocop suggestions #392
Rubocop suggestions #392
Conversation
mwolman
force-pushed
the
rubocop-suggestions
branch
from
3651295
to
a47dfa2
Compare
| @@ -0,0 +1,33 @@ | |||
| # This configuration was generated by | |||
There was a problem hiding this comment.
I like the idea of the .rubocop_todo.yml but as I understand it, it's meant for things that want to be fixed at some point (that why the "TODO"). So I wonder, is it our case here?
There was a problem hiding this comment.
@santib that's a good point.
I like the idea as well, but it kinda force us to consider every violation as "tech debt". I think we should all be aligned on this since it will impact the whole company in some way.
IMO: Every violation should be considered as tech debt. If there are no plans to fix it, does the rule make sense? is it useful? Maybe it just needs to be disabled on the main config file.
I can't think of an scenario where we want to keep the offense and still consider that rule useful.
There was a problem hiding this comment.
I agree with what you said. In general terms think there are 2 kinds of exceptions:
- Tech debt: things that should be fixed at some point
- Directory level: some directories might need different rules, especially when using DSLs like RSpec or ActiveAdmin
For the rails_api_base I think we should only have the second kind of exceptions (which are not intended to be fixed) so projects start with a clean state
db/.rubocop.yml
Outdated
| @@ -0,0 +1,8 @@ | |||
| inherit_from: | |||
There was a problem hiding this comment.
hmm I'm not really convinced about having multiples .rubocop.yml overriding each other.. I feel like just a centralized place is simpler.. not sure though
There was a problem hiding this comment.
I'm not sure that this file is in db/, I would move it to the main folder
There was a problem hiding this comment.
Agree, we should move it to the root.
We can customize the root .rubocop.yml file using Include/Exclude to enable some cops only for specific directories
.rubocop.yml
Outdated
| Layout/LineLength: | ||
| Max: 100 |
There was a problem hiding this comment.
I know it's not related to your changes but WDYT about removing this one? The default is 120 chars which makes more sense now with wider screens 🤔
| Layout/LineLength: | |
| Max: 100 |
| Lint/BinaryOperatorWithIdenticalOperands: | ||
| Enabled: false | ||
|
|
||
| Lint/DeprecatedOpenSSLConstant: | ||
| Enabled: false | ||
|
|
||
| Lint/RaiseException: | ||
| Enabled: false |
There was a problem hiding this comment.
note for the future, we should double check these cops. Might be a good idea to turn them on (default)
Description:
Some improvements/suggestions to this PR 🤗