Forem: Enable rubocop cops

[lightalloy]

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]

Can I work on this issue?

[JoppeDC]

Also interested!

[lightalloy]

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

[lightalloy]

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

[lightalloy]

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

• [x] Naming/AsciiIdentifiers
• [x] Naming/FileName

2077

[lightalloy]

• [x] Rails/Delegate
• [x] Rails/ActionFilter
• [x] Rails/FindBy
• [x] Rails/FindEach
• [x] Rails/Output
• [x] Rails/ReadWriteAttribute
• [x] Rails/ScopeArgs
• [x] Rails/Validation

[lightalloy]

• [x] Performance/CaseWhenSplat
• [x] Performance/Count
• [x] Performance/Detect
• [x] Performance/FlatMap
• [x] Performance/ReverseEach
• [x] Performance/Sample
• [x] Performance/Size
• [x] Performance/StringReplacement

2140

[lightalloy]

• [ ] Layout/AlignParameters
• [ ] Layout/ConditionPosition
• [ ] Layout/InitialIndentation

[lightalloy]

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

• [x] Lint/Void

  2130

[lightalloy]

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]

Okay sure thanks :)

[cyrillefr]

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

[Zhao-Andy]

I believe not, go for it!

[cyrillefr]

Thanks, I 'll handle it so.

[JoppeDC]

Havent gotten to it yet. So go ahead :)

On Wed, 20 Mar 2019 at 17:07, cyrillefr [e-mail] wrote:

Thanks, I 'll handle it so.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/thepracticaldev/dev.to/issues/2021#issuecomment-474904797,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AJRe5DuV0L2JcW50FsH3dJ_sHK_Af2AHks5vYlzDgaJpZM4boGAy
.

[keshavbiswa]

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

[venarius]

Will work on the Lint ones

[lightalloy]

Cops left to be fixed (from the rubocop_todo):

• [ ] Rails/HasManyOrHasOneDependent
• [x] Style/FormatString
• [x] Style/TrivialAccessors
• [x] Layout/AlignParameters
• [x] Layout/ConditionPosition
• [ ] Layout/InitialIndentation

[lightalloy]

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]

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]

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.

[lightalloy]

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 https://github.com/thepracticaldev/dev.to/issues/2021#issuecomment-477983728