Check and change some of our current rubocop configuration #456

DJA89 · 2023-09-13T20:20:17Z

Board:

Notion ticket

Description:

Checking the Rubocop rules we have set specific options to in our .rubocoop.yml

Notes:

List of changes or comments:

Layout/SpaceBeforeFirstArg: Removed exception and added AllowForAlignment so it works ok for how we use it on views.
Lint/BinaryOperatorWithIdenticalOperands: Removed, unlikely to be used but I only see positives as it removed redundancy.
Lint/DeprecatedOpenSSLConstant: Removed, it’s a deprecation warning, we shouldn’t use deprecated stuff if possible.
Lint/RaiseException: Removed, we shouldn’t raise plain Exceptions as catching them would mean catching every other kind of Exception in the same place.
Style/ArrayCoercion: Removed as it can cause problems Style/ArrayCoercion does not play nicely with Hashes rubocop/rubocop#8783.
Rails/FilePath: Removed so it’s enabled, it’s good to always use the same syntax, the default is to use arguments but I’m fine if slashes are preferred.
Style/BlockDelimiters: Removed except on specs, it feels wrong to use braces on multi line blocks, if this is how most of the company uses it nowadays then we can change it but I believe it’s the other way around. Left it on specs because it is usually how they are used for expect { }.to even if it's multiline..
Style/ExpandPathArguments: I’m not really sure what difference this makes, never used it, might be good to have it turned on just to have consistency.
Style/GlobalStdStream: Same as above.
Style/HashEachMethods: Removed as it’s faster: https://stackoverflow.com/questions/46532459/what-is-the-difference-between-keys-each-and-each-key.
Style/HashTransformKeys: Removed as we have the necessary version of ruby and it looks way less confusing.
Style/HashTransformValues: Same as above.
Style/RedundantFetchBlock: Removed as it's faster https://www.rubydoc.info/gems/rubocop/RuboCop/Cop/Style/RedundantFetchBlock.
Style/RedundantFileExtensionInRequire: I’ve removed it since I’ve never seen us use require ‘something.rb’ explicitly.
Style/RedundantRegexpCharacterClass: Removed it as it only looks positive.
Style/ReturnNil: Do we usually return nil explicitly? Not used to this but not removing it as I’m not sure what most of us do.
Style/SlicingWithRange: Removing it as we have the necessary ruby version.
Style/StringConcatenation: I personally prefer interpolation, maybe using the conservative mode, wdpt? https://www.rubydoc.info/gems/rubocop/RuboCop/Cop/Style/StringConcatenation.

Tasks:

Add each element in this format

Risk:

Preview:

guillermoap

I feel we have to discuss a lot of the changes made in this PR, some make sense to me but the majority don't.

guillermoap · 2023-09-14T15:03:56Z

.rubocop.yml

@@ -29,21 +29,11 @@ Layout/LineLength:
    - db/**/*

 Layout/SpaceBeforeFirstArg:
-  Exclude:
-    - app/views/api/**/**/*
+  AllowForAlignment: true


I don't like having aligned comments, would this limit the ability of having multi line comments that are not aligned?

I don't think this is for comments right? I think it's because in views we do

json.id user.id json.email user.email json.name user.full_name json.username user.username

which I don't like but it is what it is 🤷

This is indeed only for arguments, not comments. Layout::CommentIndentation would be the one that would modify comments

guillermoap · 2023-09-14T15:24:03Z

.rubocop.yml

 Style/BlockDelimiters:
-  EnforcedStyle: braces_for_chaining
+  Exclude:


why did we remove this config for specs? Not saying I like the braces_for_chaining but even the default one is a good config to have.

That is the idea

guillermoap · 2023-09-14T15:49:59Z

.rubocop.yml

-Lint/BinaryOperatorWithIdenticalOperands:
-  Enabled: false
-
-Lint/DeprecatedOpenSSLConstant:
-  Enabled: false
-
-Lint/RaiseException:
-  Enabled: false
-


I like this rules being enabled by default

guillermoap · 2023-09-14T15:53:42Z

.rubocop.yml

@@ -88,11 +75,9 @@ RSpec/MultipleExpectations:
 RSpec/NamedSubject:
  Enabled: false

-Style/ArrayCoercion:
-  Enabled: true


I'm 50/50 on this change.

See my note, I removed it mainly because of the issue.

I'm okay with removing this

guillermoap · 2023-09-14T15:55:23Z

.rubocop.yml

@@ -73,9 +63,6 @@ Metrics/ModuleLength:
 Metrics/PerceivedComplexity:
  Max: 12

-Rails/FilePath:
-  Enabled: false


I would leave this config

btw slashes is the default, not arguments: https://docs.rubocop.org/rubocop-rails/cops_rails.html#enforcedstyle-slashes-default

You are right. So you would leave it turned off?

I would leave this disabled as well 😄

guillermoap · 2023-09-14T16:01:44Z

.rubocop.yml

 Style/HashLikeCase:
  MinBranchesCount: 4

-Style/HashTransformKeys:
-  Enabled: false


TIL I learned about transform_keys, but again since it can generate false positives I prefer no to enable it by default

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?

We can also AutoCorrect: false so it never happens and it has to be a manual choice

guillermoap · 2023-09-14T16:02:24Z

.rubocop.yml

-Style/HashTransformKeys:
-  Enabled: false
-
-Style/HashTransformValues:


guillermoap · 2023-09-14T16:06:09Z

.rubocop.yml

-  Enabled: false
-
-Style/RedundantFileExtensionInRequire:
-  Enabled: false


guillermoap · 2023-09-14T16:06:43Z

.rubocop.yml

-  Enabled: false
-
-Style/RedundantRegexpCharacterClass:
-  Enabled: false


guillermoap · 2023-09-14T16:08:16Z

.rubocop.yml

 Style/ReturnNil:
  Enabled: true

-Style/SlicingWithRange:


As same as above given the nature of false positives this has I would keep it disabled by default

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)

sebastiancaraballo

Nice

sebastiancaraballo · 2023-10-04T14:28:20Z

.rubocop.yml

-Lint/BinaryOperatorWithIdenticalOperands:
-  Enabled: false
-
-Lint/DeprecatedOpenSSLConstant:
-  Enabled: false
-
-Lint/RaiseException:
-  Enabled: false
-


sebastiancaraballo · 2023-10-04T14:29:56Z

.rubocop.yml

@@ -73,9 +63,6 @@ Metrics/ModuleLength:
 Metrics/PerceivedComplexity:
  Max: 12

-Rails/FilePath:
-  Enabled: false


I would leave this disabled as well 😄

sebastiancaraballo · 2023-10-04T14:32:00Z

.rubocop.yml

@@ -88,11 +75,9 @@ RSpec/MultipleExpectations:
 RSpec/NamedSubject:
  Enabled: false

-Style/ArrayCoercion:
-  Enabled: true


I'm okay with removing this

sebastiancaraballo · 2023-10-04T14:35:11Z

.rubocop.yml

@@ -103,35 +88,14 @@ Style/ExpandPathArguments:
 Style/GlobalStdStream:
  Enabled: false

-Style/HashEachMethods:


https://docs.rubocop.org/rubocop/cops_style.html#safety-22

Check and change some of our current rubocop configuration

565364b

DJA89 requested review from santib, guillermoap, JulianPasquale, mwolman and TimoPeraza September 13, 2023 20:20

guillermoap reviewed Sep 14, 2023

View reviewed changes

sebastiancaraballo reviewed Oct 4, 2023

View reviewed changes

santib closed this Apr 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check and change some of our current rubocop configuration #456

Check and change some of our current rubocop configuration #456

DJA89 commented Sep 13, 2023

guillermoap left a comment

guillermoap Sep 14, 2023

santib Sep 14, 2023

DJA89 Sep 14, 2023

guillermoap Sep 14, 2023

DJA89 Sep 14, 2023

guillermoap Sep 14, 2023

DJA89 Sep 14, 2023

sebastiancaraballo Oct 4, 2023

guillermoap Sep 14, 2023

DJA89 Sep 14, 2023

sebastiancaraballo Oct 4, 2023

guillermoap Sep 14, 2023

guillermoap Sep 14, 2023

DJA89 Sep 14, 2023

sebastiancaraballo Oct 4, 2023

guillermoap Sep 14, 2023

santib Sep 14, 2023

DJA89 Sep 14, 2023

guillermoap Sep 14, 2023

guillermoap Sep 14, 2023

guillermoap Sep 14, 2023

guillermoap Sep 14, 2023

DJA89 Sep 14, 2023

sebastiancaraballo left a comment

sebastiancaraballo Oct 4, 2023

sebastiancaraballo Oct 4, 2023

sebastiancaraballo Oct 4, 2023

sebastiancaraballo Oct 4, 2023

Check and change some of our current rubocop configuration #456

Check and change some of our current rubocop configuration #456

Conversation

DJA89 commented Sep 13, 2023

Board:

Description:

Notes:

Tasks:

Risk:

Preview:

guillermoap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebastiancaraballo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment