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 Style/ReturnNil cop #529

Merged
merged 2 commits into from
Apr 19, 2023
Merged

Enable Style/ReturnNil cop #529

merged 2 commits into from
Apr 19, 2023

Conversation

larouxn
Copy link
Contributor

@larouxn larouxn commented Apr 13, 2023

Proposing we enable Style/ReturnNil to enforce return if/unless condition style over return nil unless condition as the nil component is redundant/unnecessary. Let's make this consistent.

https://www.rubydoc.info/gems/rubocop/RuboCop/Cop/Style/ReturnNil

@larouxn larouxn self-assigned this Apr 13, 2023
@github-actions github-actions bot added the config change Changes the Rubocop config by enabling, disabling, or reconfiguring one or many cops label Apr 13, 2023
larouxn and others added 2 commits April 13, 2023 12:17
This automated commit dumps the contents of the full RuboCop config.
[dependabot skip]
@larouxn larouxn marked this pull request as ready for review April 13, 2023 16:22
@larouxn larouxn requested a review from a team as a code owner April 13, 2023 16:22
Copy link
Contributor

@sambostock sambostock left a comment

Choose a reason for hiding this comment

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

I'm not necessarily opposed to this, but some people might prefer the explicitness. 🤷 I do think consistency would be nice though, and I can't see us enabling the opposite style (i.e. enforcing return nil).

Can you run Rubocop against Core with this setting and see how many offenses there are?

@larouxn
Copy link
Contributor Author

larouxn commented Apr 13, 2023

I'm not necessarily opposed to this, but some people might prefer the explicitness. 🤷 I do think consistency would be nice though, and I can't see us enabling the opposite style (i.e. enforcing return nil).

Yeah I'm sure some peep would prefer explicit nil returns but I think in general simply returning based on a condition (implied nil) is equally valid and if we are to choose one or the other for consistency I think return condition is better than return nil condition.

Can you run Rubocop against Core with this setting and see how many offenses there are?

Cannot seem to swap the rubocop-shopify gem nor add the rule to rubocop.yml as I'm having network congestion issues given its core and I'm on train grade WiFi. That said, I was able to simply query for all return nil and there are 1917 offenses.

Screenshot 2023-04-13 at 13 27 45

Copy link
Member

@rafaelfranca rafaelfranca left a comment

Choose a reason for hiding this comment

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

I'm being more conservative on enabling cops that can have impact on existing code and doesn't add much value other than consistency, but I'm ok with this cop being enabled.

@larouxn
Copy link
Contributor Author

larouxn commented Apr 17, 2023

👋 @sambostock, I was finally able to run this cop against core. 1594 infractions. Results here.

@larouxn larouxn requested a review from sambostock April 18, 2023 23:11
@larouxn larouxn merged commit 667601c into main Apr 19, 2023
@larouxn larouxn deleted the enable_return_nil_cop branch April 19, 2023 05:38
@shopify-shipit shopify-shipit bot temporarily deployed to rubygems June 9, 2023 20:22 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
config change Changes the Rubocop config by enabling, disabling, or reconfiguring one or many cops
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants