Skip to content

eng: loosen rubocop restrictions ECOMM-7882 #50

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

Open
tshedor wants to merge 4 commits into
base: master
Choose a base branch
from
Open

eng: loosen rubocop restrictions ECOMM-7882 #50

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d0ac31e
eng: loosen rubocop restrictions ECOMM-7882
tshedor Feb 6, 2026
6cd7f4c
too far
tshedor Feb 6, 2026
866fe8e
enable multiple restrictions
tshedor Feb 6, 2026
9489b20
bump version
tshedor Feb 6, 2026
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
22 changes: 17 additions & 5 deletions config/default.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,21 +77,27 @@ Layout/SpaceInsideBlockBraces:
Layout/SpaceInsideHashLiteralBraces:
EnforcedStyle: no_space

Metrics/AbcSize:
Max: 25

# Exclude RSpec
Metrics/BlockLength:
Exclude:
- "spec/factories/**/*.rb"
AllowedMethods: ['describe', 'context']

# Too short methods lead to extraction of single-use methods, which can make
# the code easier to read (by naming things), but can also clutter the class
Metrics/MethodLength:
Max: 20

# The guiding principle of classes is SRP, SRP can't be accurately measured by LoC
Metrics/ClassLength:
Max: 1500

Metrics/CyclomaticComplexity:
Max: 12

# Too short methods lead to extraction of single-use methods, which can make
# the code easier to read (by naming things), but can also clutter the class
Metrics/MethodLength:
Max: 40

# Check with yard instead.
Style/DocumentationMethod:
Enabled: false
Expand Down Expand Up @@ -290,6 +296,12 @@ Rails/I18nLocaleTexts:
RSpec/ContextWording:
Enabled: false

RSpec/ExampleLength:
Max: 20

RSpec/MultipleExpectations:
Enabled: false
Copy link
Member

@8FAX 8FAX Feb 6, 2026

Choose a reason for hiding this comment

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

I think this is set to false by default right, are we just adding it for clarity that more then 1 expect per it block is wrong?

Copy link
Author

@tshedor tshedor Feb 6, 2026

Choose a reason for hiding this comment

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

It's enabled by default

Copy link
Member

@8FAX 8FAX Feb 6, 2026

Choose a reason for hiding this comment

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

So by setting this false we are allowing multiple expects per it block?

Copy link
Author

@tshedor tshedor Feb 6, 2026

Choose a reason for hiding this comment

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

Yes

Copy link
Member

@tmyrick tmyrick Feb 6, 2026

Choose a reason for hiding this comment

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

I'm not in favor of this one but not a huge deal

Copy link
Member

@tmyrick tmyrick Feb 6, 2026

Choose a reason for hiding this comment

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

rspec doesn't do a good job at letting you know which expectation failed when there are multiple

Copy link
Member

@8FAX 8FAX Feb 6, 2026

Choose a reason for hiding this comment

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

I agree with Travis here, I think if we're going to loosen restrictions on application logic, we should still try to maintain the current test restrictions until we have a better idea of how this change actually will affect the code output.

Copy link
Author

@tshedor tshedor Feb 6, 2026

Choose a reason for hiding this comment

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

@tmyrick @8FAX This is genuinely my least favorite lint and the one I disable the most. But if you're both on board with keeping it I'll remove.

Copy link
Member

@8FAX 8FAX Feb 6, 2026

Choose a reason for hiding this comment

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

Thanks for removing this, I think we may get to a point where this can be removed, but for now I think its helpful. I am more then happy to talk about this later on if we want to move to remove this.

Copy link
Member

@evan-waters evan-waters Feb 6, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I somewhat agree with the intended purpose of this lint. It prevents you from having hidden failures in a single test, but I do not necessarily agree with it as a style rule. I favor using :aggregate_failures on my test block to get around this lint. The rule allows this without having to ignore.


RSpec/AlignLeftLetBrace:
Enabled: true

Expand Down