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.

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

Enable rubocop cops #2021

Closed
lightalloy opened this issue Mar 11, 2019 · 22 comments

Comments

@lightalloy
Copy link
Collaborator

commented Mar 11, 2019

Is your feature request related to a problem? Please describe.
Currently, many rubocop cops are disabled (see .rubocop.yml), I think it would be beneficial to enable most of them. In such a way at least the new code will follow the guidelines.

Describe the solution you'd like
I like the way how we dealt with the views linting problems (#1842)
The similar approach is possible for the ruby code & the rubocop

The workflow I suggest:

  • enable 1 cop from the .rubocop.yml
  • fix related issues if possible
  • if it's not possible (or optimal), add exceptions to the .rubocop_todo.yml (there're already some examples)
  • make a pull request

For some cops, there are no violations in the code, so they could just be enabled.
I can prepare a list of cops to enable if this issue gets approved.

@keshavbiswa

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

Can I work on this issue?

@JoppeDC

This comment has been minimized.

Copy link

commented Mar 12, 2019

Also interested!

@lightalloy

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 13, 2019

@keshavbiswa @JoppeDC you're welcome.
I'll be posting lists of the cops you can work on.

@lightalloy

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 13, 2019

  • Style/Alias
  • Style/ArrayJoin
  • Style/Attr
  • Style/CaseEquality
  • Style/CharacterLiteral
  • Style/ColonMethodCall
  • Style/CommentAnnotation
  • Style/PreferredHashMethods
  • Style/DoubleNegation
  • Style/EachWithObject
  • Style/EmptyLiteral
  • Style/EvenOdd
  • Style/GuardClause
  • Style/IfUnlessModifier
  • Style/IfWithSemicolon
  • Style/Lambda
  • Style/LambdaCall
  • Style/LineEndConcatenation
  • Style/ModuleFunction
  • Style/NegatedIf
  • Style/NegatedWhile
  • Style/NilComparison
  • Style/Not
  • Style/NumericLiterals
  • Style/OneLineConditional
  • Style/PercentLiteralDelimiters
  • Style/PerlBackrefs
  • Style/Proc
  • Style/RaiseArgs
  • Style/SelfAssignment
  • Style/SingleLineMethods
  • Style/SpecialGlobalVars
  • Style/VariableInterpolation
  • Style/WhenThen
  • Style/WhileUntilModifier
  • Style/WordArray
@lightalloy

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 13, 2019

I suppose there're no violations to these cops, so they can just be enabled:

  • Naming/AsciiIdentifiers
  • Naming/FileName

#2077

@lightalloy

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 13, 2019

  • Rails/Delegate
  • Rails/ActionFilter
  • Rails/FindBy
  • Rails/FindEach
  • Rails/Output
  • Rails/ReadWriteAttribute
  • Rails/ScopeArgs
  • Rails/Validation
@lightalloy

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 13, 2019

  • Performance/CaseWhenSplat
  • Performance/Count
  • Performance/Detect
  • Performance/FlatMap
  • Performance/ReverseEach
  • Performance/Sample
  • Performance/Size
  • Performance/StringReplacement

#2140

@lightalloy

This comment was marked as outdated.

Copy link
Collaborator Author

commented Mar 13, 2019

  • Layout/AlignParameters
  • Layout/ConditionPosition
  • Layout/InitialIndentation
@lightalloy

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 13, 2019

  • Lint/AmbiguousOperator
  • Lint/AmbiguousRegexpLiteral
  • Lint/AssignmentInCondition
  • Lint/CircularArgumentReference
  • Lint/DeprecatedClassMethods
  • Lint/DuplicatedKey
  • Lint/EachWithObjectArgument
  • Lint/ElseLayout
  • Lint/FormatParameterMismatch
  • Lint/LiteralAsCondition
  • Lint/LiteralInInterpolation
  • Lint/Loop
  • Lint/NestedMethodDefinition
  • Lint/ParenthesesAsGroupedExpression
  • Lint/RequireParentheses
  • Lint/UnderscorePrefixedVariableName
  • Lint/UnneededCopDisableDirective
  • Lint/Void

#2130

@lightalloy

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 13, 2019

Please, notify about the cops you're working on in this thread.
If you have any problems or questions, feel free to ask them in this thread or in your (draft) pull request.

@keshavbiswa

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

Okay sure thanks :)

@cyrillefr

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

Hello, I would like to participate too.
Is it too late to do the Performance cops ?

@Zhao-Andy

This comment has been minimized.

Copy link
Collaborator

commented Mar 20, 2019

I believe not, go for it!

@cyrillefr

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

Thanks, I 'll handle it so.

@JoppeDC

This comment has been minimized.

Copy link

commented Mar 20, 2019

@keshavbiswa

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

@cyrillefr Go for it! I will work on Layout Cops 😄

@venarius

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

Will work on the Lint ones

@venarius venarius referenced this issue Mar 20, 2019
0 of 5 tasks complete
cyrillefr added a commit to cyrillefr/dev.to that referenced this issue Mar 20, 2019
Enabled Performance/CaseWhenSplat Cop
By deleting lines in rubocop.yml

Fixes thepracticaldev#2021
cyrillefr added a commit to cyrillefr/dev.to that referenced this issue Mar 20, 2019
Enabled Performance/Count Cop
By deleting lines in rubocop.yml

Fixes thepracticaldev#2021
cyrillefr added a commit to cyrillefr/dev.to that referenced this issue Mar 20, 2019
Enabled Performance/Detect Cop
By deleting lines in rubocop.yml

Fixes thepracticaldev#2021
cyrillefr added a commit to cyrillefr/dev.to that referenced this issue Mar 20, 2019
Enabled Performance/FlatMap Cop
By deleting lines in rubocop.yml

Fixes thepracticaldev#2021
cyrillefr added a commit to cyrillefr/dev.to that referenced this issue Mar 20, 2019
Enabled Performance/ReverseEach Cop
By deleting lines in rubocop.yml

Fixes thepracticaldev#2021
cyrillefr added a commit to cyrillefr/dev.to that referenced this issue Mar 20, 2019
Enabled Performance/Sample Cop
By deleting lines in rubocop.yml

Fixes thepracticaldev#2021
cyrillefr added a commit to cyrillefr/dev.to that referenced this issue Mar 20, 2019
Enabled Performance/Size Cop
By deleting lines in rubocop.yml

Fixes thepracticaldev#2021
cyrillefr added a commit to cyrillefr/dev.to that referenced this issue Mar 20, 2019
Enabled Performance/StringReplacement Cop
By deleting lines in rubocop.yml
And refactoring accordingly

Fixes thepracticaldev#2021
@cyrillefr cyrillefr referenced this issue Mar 20, 2019
2 of 7 tasks complete
@lightalloy lightalloy referenced this issue Mar 28, 2019
1 of 1 task complete
@lightalloy

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 29, 2019

Cops left to be fixed (from the rubocop_todo):

  • Rails/HasManyOrHasOneDependent
  • Style/FormatString
  • Style/TrivialAccessors
  • Layout/AlignParameters
  • Layout/ConditionPosition
  • Layout/InitialIndentation
@lightalloy

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 29, 2019

I think these cops should be enabled + exclude files that contain violations + configure max values where applicable:

  • Metrics/ClassLength
  • Metrics/ModuleLength
  • Metrics/AbcSize
  • Metrics/CyclomaticComplexity
  • Style/GlobalVars
  • Metrics/MethodLength
  • Metrics/ParameterLists
@cyrillefr

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

Hello,

I would like to take care of the first group of cops(rubocop_todo).
But before proceeding, I would like to put what I intend to do:

Rails/HasManyOrHasOneDependent
|→ 28 offenses for not specifying a :dependent option. To discard these offenses, I can add a :dependent :nullify(default value for :dependent).

Rails/SkipsModelValidations
|→ 45 offenses on spec files because update_column skips validation. I can change to update.
|→ 104 offenses on other files

  • update_column skips validation: same as above
  • touch skips validation: no other solution here than adding touch method to the whitelist (the docs indicates so).
  • update_all skips validation: I can use the batch mode and update each objects rather than whitelisting update_all(I lack the functional knowledge to choose the best option).

Layout: 177 offenses: I can fix with the automatic fix option of rubocop.

NB: I may be behind a few commits, so the number might be a little bit inaccurate, but of same order.
I did not ran any tests, so I have got no idea of new errors that may arise from these changes.

So, tell me if I may proceed this way.

@lightalloy

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 5, 2019

Hey @cyrillefr
I revised the cops list, and think that I should first discuss enabling and fixing Rails/HasManyOrHasOneDependent with the team.
As for the Rails/SkipsModelValidations now I think it shouldn't be enabled. Running some validations need running SQL-queries which will affect performance. update_all and update_column are fine if we use them consciously.
Fixing Layout cops should be straightforward, so feel free to fix them.

@rhymes rhymes referenced this issue Apr 5, 2019
2 of 6 tasks complete
@lightalloy lightalloy referenced this issue Apr 8, 2019
1 of 1 task complete
@lightalloy

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 8, 2019

I made small updates to the config.
For now:

  • Rails/HasManyOrHasOneDependent should be safe to be enabled, :dependent should be set to :nullify (as @cyrillefr suggested). I suggest making a separate pr for this fix.
  • Layout/InitialIndentation is still available to be fixed
  • this comment is also relevant #2021 (comment)
@rhymes rhymes referenced this issue Apr 8, 2019
3 of 7 tasks complete
benhalpern added a commit that referenced this issue Apr 8, 2019
Improve rubocop config #2021 (#2324)
* Methods that skip validations are fine

* Fix gems order

* Enable layout cops that have no violations
cyrillefr added a commit to cyrillefr/dev.to that referenced this issue Apr 8, 2019
Enable Layout/AlignParameters
Check if the parameters on a multi-line method call or definition are aligned.

Resolves: thepracticaldev#2021
@cyrillefr cyrillefr referenced this issue Apr 8, 2019
2 of 7 tasks complete
benhalpern added a commit that referenced this issue Apr 8, 2019
Enable Layout/AlignParameters (#2340)
Check if the parameters on a multi-line method call or definition are aligned.

Resolves: #2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.