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
Check and change some of our current rubocop configuration #456
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,21 +29,11 @@ Layout/LineLength: | |
- db/**/* | ||
|
||
Layout/SpaceBeforeFirstArg: | ||
Exclude: | ||
- app/views/api/**/**/* | ||
AllowForAlignment: true | ||
|
||
Lint/AmbiguousBlockAssociation: | ||
AllowedMethods: change | ||
|
||
Lint/BinaryOperatorWithIdenticalOperands: | ||
Enabled: false | ||
|
||
Lint/DeprecatedOpenSSLConstant: | ||
Enabled: false | ||
|
||
Lint/RaiseException: | ||
Enabled: false | ||
|
||
Comment on lines
-38
to
-46
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like this rules being enabled by default There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do too There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice |
||
Metrics/AbcSize: | ||
Max: 15 | ||
Exclude: | ||
|
@@ -73,9 +63,6 @@ Metrics/ModuleLength: | |
Metrics/PerceivedComplexity: | ||
Max: 12 | ||
|
||
Rails/FilePath: | ||
Enabled: false | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would leave this config There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. btw slashes is the default, not arguments: https://docs.rubocop.org/rubocop-rails/cops_rails.html#enforcedstyle-slashes-default There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are right. So you would leave it turned off? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would leave this disabled as well 😄 |
||
|
||
Rails/SaveBang: | ||
Enabled: true | ||
|
||
|
@@ -88,11 +75,9 @@ RSpec/MultipleExpectations: | |
RSpec/NamedSubject: | ||
Enabled: false | ||
|
||
Style/ArrayCoercion: | ||
Enabled: true | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm 50/50 on this change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See my note, I removed it mainly because of the issue. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm okay with removing this |
||
|
||
Style/BlockDelimiters: | ||
EnforcedStyle: braces_for_chaining | ||
Exclude: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why did we remove this config for specs? Not saying I like the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
- spec/**/* | ||
|
||
Style/Documentation: | ||
Enabled: false | ||
|
@@ -103,35 +88,14 @@ Style/ExpandPathArguments: | |
Style/GlobalStdStream: | ||
Enabled: false | ||
|
||
Style/HashEachMethods: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given that this cop is unsafe I think it's a good idea to leave it in false There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, I didn't know it was unsafe, do you have a link? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
Enabled: false | ||
|
||
Style/HashLikeCase: | ||
MinBranchesCount: 4 | ||
|
||
Style/HashTransformKeys: | ||
Enabled: false | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TIL I learned about transform_keys, but again since it can generate false positives I prefer no to enable it by default There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://docs.rubocop.org/rubocop/usage/auto_correct.html#safe-auto-correct maybe we don't need to worry too much about the unsafe cops since we can use a safe autocorrect? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can also |
||
|
||
Style/HashTransformValues: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here |
||
Enabled: false | ||
|
||
Style/ModuleFunction: | ||
Enabled: false | ||
|
||
Style/RedundantFetchBlock: | ||
Enabled: false | ||
|
||
Style/RedundantFileExtensionInRequire: | ||
Enabled: false | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍🏻 |
||
|
||
Style/RedundantRegexpCharacterClass: | ||
Enabled: false | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍🏻 |
||
|
||
Style/ReturnNil: | ||
Enabled: true | ||
|
||
Style/SlicingWithRange: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As same as above given the nature of false positives this has I would keep it disabled by default There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was getting my info from rubydoc.info which did not mention some of these false positives, I see docs.rubocop.org is better documented (as it should) |
||
Enabled: false | ||
|
||
Style/StringConcatenation: | ||
Enabled: false |
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 don't like having aligned comments, would this limit the ability of having multi line comments that are not aligned?
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 don't think this is for comments right? I think it's because in views we do
which I don't like but it is what it is 🤷
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.
This is indeed only for arguments, not comments.
Layout::CommentIndentation
would be the one that would modify comments