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.

Sign up for GitHub

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

Jump to bottom

Sensible configuration for rubocop #560

Closed
wants to merge 6 commits into from
Closed

Sensible configuration for rubocop #560

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8664245
use rubocop settings from shopify and set some rules where we deviate
fosterfarrell9 Nov 12, 2023
5524055
remove some of the deviatons from shopify rubocop settings
fosterfarrell9 Nov 14, 2023
c4187a4
chage maximum line length settings, remove empty newline
fosterfarrell9 Nov 15, 2023
2292a63
update rubocop and rubocop-shopify versions
fosterfarrell9 Nov 15, 2023
4db8503
change setting for ClassMethodsDefinitions
fosterfarrell9 Nov 15, 2023
1e50c50
add missing new line at the end of file
fosterfarrell9 Nov 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
343 changes: 7 additions & 336 deletions .rubocop.yml
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

What about Style/ClassMethodsDefinitions, do we really want that?

It will enforce this affecting us 132 times in our codebase. I'd rather go with EnforcedStyle: def_self.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agree and done.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I also updated the wiki page correspondingly.

Copy link
Member

Choose a reason for hiding this comment

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

Ok thanks. I wonder: if we add more custom things to the .rubocop.yml file, maybe it's a bit tedious to keep it synced with the Wiki? Except for the link to the style guide, I do not see the benefit of listing every change we make in the Wiki.

One could just open the .rubocop.yml file (maybe link to that in the Wiki?) or even run rubocop on the local code changes to directly see where one violates our styles. In the best case, the plugin for VSCode also adds blue squiggly lines to indicate style violations. In my setup, it even autocorrects these (if the autocorrections are safe) whenever I save the file.

If you argue that the Wiki is a place where we could add additional comments for why we chose to do something, we could also have that in the Details section.

Copy link
Member

Choose a reason for hiding this comment

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

Shopify also sets CountKeywordArgs to false which enforces this behavior. I prefer to set it to true as is the RuboCop default.

Copy link
Member

Choose a reason for hiding this comment

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

Shopify disabled Lint/UnreachableLoop. I vote for having it enabled.

Copy link
Member

Choose a reason for hiding this comment

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

Style/CaseLikeIf is disabled in Shopify. What do you think about adding a Severity: refactor to it?

Copy link
Member

Choose a reason for hiding this comment

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

What's your opinion on Style/GuardClause which is disabled in Shopify. I don't have a strong opinion on this but remember you once suggested to use unless, so you probably want it enabled?

Copy link
Member

Choose a reason for hiding this comment

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

I strongly vote to enable the Style/DoubleNegation cop.

Copy link
Member

Choose a reason for hiding this comment

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

Instead of disabling Style/EmptyMethod, we should enforce our preferred style, which is EnforcedStyle: expanded.

Copy link
Member

Choose a reason for hiding this comment

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

I strongly suggest enabling Style/NegatedUnless, otherwise you have to write down your own truth table to find out when the if statement is true.

Copy link
Member

Choose a reason for hiding this comment

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

I strongly suggest enabling Style/NegatedIfElseCondition.

Copy link
Member

Choose a reason for hiding this comment

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

I suggest enabling the cop Style/YodaCondition with the default EnforcedStyle.

Copy link
Member

Choose a reason for hiding this comment

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

I strongly suggest enabling the cop Style/TrailingMethodEndStatement. Why would one disable that? I see no scenario where that could be a useful thing to do.

Copy link
Member

Choose a reason for hiding this comment

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

I strongly suggest enabling the cop Style/TrailingMethodEndStatement.

Copy link
Member

Choose a reason for hiding this comment

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

I prefer to have Style/SwapValues enabled as this makes the intention of what is really done in the code clearer.

Copy link
Member

Choose a reason for hiding this comment

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

I suggest not disabling any security-related RuboCop cop like Shopify does. This is since I don't feel like I'm in the expert position to judge if we don't need this, so I'd prefer having this as an error that signifies to me I really have to watch out and should probably use a different way to do things (since smart people already figured this could be a potential security issue).

I even suggest writing the following to enable all security-related Rubocop cops.

Security:
  Enabled: true

Copy link
Member

Choose a reason for hiding this comment

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

I don't see a use-case for self-assignment, so I'd suggest to enable the cop Lint/SelfAssignment.

Copy link
Member

Choose a reason for hiding this comment

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

I suggest enabling the cop Lint/EmptyBlock.

Copy link
Member

Choose a reason for hiding this comment

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

I suggest enabling the cop Lint/DuplicateRequire.

Original file line number Diff line number Diff line change
@@ -1,341 +1,12 @@
require:
- rubocop-packaging
- rubocop-performance
- rubocop-rails

AllCops:
TargetRubyVersion: 3.0
# RuboCop has a bunch of cops enabled by default. This setting tells RuboCop
# to ignore them, so only the ones explicitly set in this file are enabled.
DisabledByDefault: false
Exclude:
- '**/tmp/**/*'
- '**/templates/**/*'
- '**/vendor/**/*'
- 'actionpack/lib/action_dispatch/journey/parser.rb'
- 'actionmailbox/test/dummy/**/*'
- 'actiontext/test/dummy/**/*'
- '**/node_modules/**/*'

Performance:
Exclude:
- '**/test/**/*'

# Prefer assert_not over assert !
Rails/AssertNot:
Include:
- '**/test/**/*'

# Prefer assert_not_x over refute_x
Rails/RefuteMethods:
Include:
- '**/test/**/*'

Rails/IndexBy:
Enabled: true

Rails/IndexWith:
Enabled: true

# Prefer &&/|| over and/or.
Style/AndOr:
Enabled: true

Layout/ArgumentAlignment:
Enabled: true

Layout/ArrayAlignment:
Enabled: true

Layout/BlockAlignment:
Enabled: true

# Align `when` with `case`.
Layout/CaseIndentation:
Enabled: true

Layout/ClosingHeredocIndentation:
Enabled: true

# Align comments with method definitions.
Layout/CommentIndentation:
Enabled: true

Layout/ElseAlignment:
Enabled: true

# Align `end` with the matching keyword or starting expression except for
# assignments, where it should be aligned with the LHS.
Layout/EndAlignment:
Enabled: true
EnforcedStyleAlignWith: variable
AutoCorrect: true

Layout/EmptyLines:
Enabled: true

Layout/EmptyLineAfterMagicComment:
Enabled: true

Layout/EmptyLinesAroundAccessModifier:
Enabled: true

Layout/EmptyLinesAroundBlockBody:
Enabled: true

# In a regular class definition, no empty lines around the body.
Layout/EmptyLinesAroundClassBody:
Enabled: true

# In a regular method definition, no empty lines around the body.
Layout/EmptyLinesAroundMethodBody:
Enabled: true

# In a regular module definition, no empty lines around the body.
Layout/EmptyLinesAroundModuleBody:
Enabled: true

Layout/ExtraSpacing:
Enabled: true
inherit_gem:
rubocop-shopify: rubocop.yml

# Hard limit is 100 characters, soft limit is 80 characters
Layout/LineLength:
Max: 80

# Use Ruby >= 1.9 syntax for hashes. Prefer { a: :b } over { :a => :b }.
Style/HashSyntax:
Enabled: true

Layout/FirstArgumentIndentation:
Enabled: true

Layout/HashAlignment:
Enabled: true

# Method definitions after `private` or `protected` isolated calls need one
# extra level of indentation.
Layout/IndentationConsistency:
Enabled: true
EnforcedStyle: indented_internal_methods

# Two spaces, no tabs (for indentation).
Layout/IndentationWidth:
Enabled: true

Layout/LeadingCommentSpace:
Enabled: true

Layout/MultilineOperationIndentation:
Enabled: true

Layout/SpaceAfterColon:
Enabled: true

Layout/SpaceAfterComma:
Enabled: true

Layout/SpaceAfterSemicolon:
Enabled: true

Layout/SpaceAroundEqualsInParameterDefault:
Enabled: true

Layout/SpaceAroundKeyword:
Enabled: true

Layout/SpaceBeforeComma:
Enabled: true

Layout/SpaceBeforeComment:
Enabled: true

Layout/SpaceBeforeFirstArg:
Enabled: true

Layout/SpaceInsideArrayLiteralBrackets:
Enabled: true

Style/BlockDelimiters:
Enabled: true

Style/BlockComments:
Enabled: true

Style/ConditionalAssignment:
Enabled: true

Style/DefWithParentheses:
Enabled: true

Style/Documentation:
Enabled: true

# Defining a method with parameters needs parentheses.
Style/MethodDefParentheses:
Enabled: true

Style/MethodCallWithoutArgsParentheses:
Enabled: true
Max: 100

Style/FrozenStringLiteralComment:
Enabled: true
EnforcedStyle: always
Exclude:
- 'actionview/test/**/*.builder'
- 'actionview/test/**/*.ruby'
- 'actionpack/test/**/*.builder'
- 'actionpack/test/**/*.ruby'
- 'activestorage/db/migrate/**/*.rb'
- 'activestorage/db/update_migrate/**/*.rb'
- 'actionmailbox/db/migrate/**/*.rb'
- 'actiontext/db/migrate/**/*.rb'

Style/NumericLiterals:
Enabled: true

Style/NumericPredicate:
Enabled: true

Style/RedundantFreeze:
Enabled: true

Style/SymbolProc:
Enabled: true

Style/SymbolArray:
Enabled: false

# Use `foo {}` not `foo{}`.
Layout/SpaceBeforeBlockBraces:
Enabled: true

# Use `foo { bar }` not `foo {bar}`.
Layout/SpaceInsideBlockBraces:
Enabled: true
EnforcedStyleForEmptyBraces: space

# Use `{ a: 1 }` not `{a:1}`.
Layout/SpaceInsideHashLiteralBraces:
Enabled: true

Layout/SpaceInsideParens:
Enabled: true

# Check quotes usage according to lint rule below.
Style/StringLiterals:
Enabled: true
EnforcedStyle: single_quotes

# Detect hard tabs, no hard tabs.
Layout/IndentationStyle:
Enabled: true

# Empty lines should not have any spaces.
Layout/TrailingEmptyLines:
Enabled: true

# No trailing whitespace.
Layout/TrailingWhitespace:
Enabled: true

# Use quotes for string literals when they are enough.
Style/RedundantPercentQ:
Enabled: true

Lint/AmbiguousOperator:
Enabled: true

Lint/AmbiguousRegexpLiteral:
Enabled: true

Lint/ErbNewArguments:
Enabled: true

# Use my_method(my_arg) not my_method( my_arg ) or my_method my_arg.
Lint/RequireParentheses:
Enabled: true

Lint/ShadowingOuterLocalVariable:
Enabled: true

Lint/RedundantStringCoercion:
Enabled: true

Lint/UriEscapeUnescape:
Enabled: true

Lint/UselessAssignment:
Enabled: true

Lint/DeprecatedClassMethods:
Enabled: true

Naming/VariableNumber:
Enabled: true

Style/ParenthesesAroundCondition:
Enabled: true

Style/HashTransformKeys:
Enabled: true

Style/HashTransformValues:
Enabled: true

Style/RedundantBegin:
Enabled: true

Style/RedundantReturn:
Enabled: true
AllowMultipleReturnValues: true

Style/Semicolon:
Enabled: true
AllowAsExpressionSeparator: true

# Prefer Foo.method over Foo::method
Style/ColonMethodCall:
Enabled: true

Style/TrivialAccessors:
Enabled: true

Performance/FlatMap:
Enabled: true

Performance/RedundantMerge:
Enabled: true

Performance/StartWith:
Enabled: true

Performance/EndWith:
Enabled: true

Performance/RegexpMatch:
Enabled: true

Performance/ReverseEach:
Enabled: true

Performance/UnfreezeString:
Enabled: true

Performance/DeletePrefix:
Enabled: true

Performance/DeleteSuffix:
Enabled: true

Metrics/ModuleLength:
Enabled: false

Metrics/ClassLength:
Enabled: false

Metrics/MethodLength:
Exclude:
- 'app/abilities/**/*'
EnforcedStyle: never

fosterfarrell9 marked this conversation as resolved.
Show resolved Hide resolved
Metrics/AbcSize:
Exclude:
- 'app/abilities/**/*'
Style/ClassMethodsDefinitions:
EnforcedStyle: self_class
7 changes: 2 additions & 5 deletions Gemfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,11 +118,8 @@ group :development, :docker_development do
# Spring speeds up development by keeping your application running in the background. Read more: https://github.com/rails/spring
gem "spring"
gem "spring-watcher-listen", "~> 2.0.0"
gem "rubocop", "~> 1.50", require: false
gem "rubocop-packaging", require: false
gem "rubocop-performance", require: false
gem "rubocop-rails", require: false
gem "erb_lint", require: false
gem "rubocop", "~> 1.57", require: false
gem "rubocop-shopify", "~> 2.14", require: false
gem "pgreset"
gem "marcel"
# gem 'bullet'
Expand Down
Loading
Loading