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.

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

Jump to bottom

tools: enable no-else-return lint rule #32667

Closed
wants to merge 1 commit into from
Closed

tools: enable no-else-return lint rule #32667

wants to merge 1 commit into from

Conversation

lpinca
Copy link
Member

@lpinca lpinca commented Apr 5, 2020

Refs: #32644
Refs: #32662

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Apr 5, 2020
@lpinca lpinca added the blocked PRs that are blocked by other issues or PRs. label Apr 5, 2020
@lpinca
Copy link
Member Author

lpinca commented Apr 5, 2020

Blocked to allow #32644 and #32662 to land first.

@lpinca lpinca force-pushed the enable/no-else-return branch 4 times, most recently from 69890a3 to 73b15aa Compare April 5, 2020 12:48
@targos
Copy link
Member

targos commented Apr 6, 2020

I won't block this, but I personally prefer the way the current code is written.

@devsnek
Copy link
Member

devsnek commented Apr 6, 2020

I think many of these cases are clearer when written including the else block.

@lpinca
Copy link
Member Author

lpinca commented Apr 6, 2020

I have no strong opinion, this is just to have consistency and to not have PRs like #32644 and #32662.

@lpinca
Copy link
Member Author

lpinca commented Apr 8, 2020

Before closing I'll will rework the PR with the allowElseIf option set to true. It should remove most of the churn and the controversial changes.

@lpinca lpinca force-pushed the enable/no-else-return branch 2 times, most recently from dd909ba to e404e2c Compare April 8, 2020 17:09
@lpinca
Copy link
Member Author

lpinca commented Apr 8, 2020

@targos @devsnek @addaleax PTAL.

@addaleax
Copy link
Member

addaleax commented Apr 8, 2020

@lpinca No strong opinions here. I like if (...) { return ...; } else { return ...; } if there is some kind of parallelism between the two statements, but ultimately, I really don’t care enough. As long as this is a lint rule that can work with make lint-js-fix, I’m good.

@targos
Copy link
Member

targos commented Apr 8, 2020

same as @addaleax here

@lpinca
Copy link
Member Author

lpinca commented Apr 8, 2020 •
edited
Loading

The only reason for me is to prevent PRs with only cosmetic changes (unless we want them). In my opinion it's better for contributors to have a linter error than a PR rejected with a reason like "Cosmetic only changes".

Also cc @Trott who commented saying that he preferred to close this but then removed the comment.

BridgeAR
BridgeAR approved these changes Apr 8, 2020
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

LGTM. It reduces the number of code lines and the indentation which often improves the readability of the code.

@Trott
Copy link
Member

Trott commented Apr 8, 2020

Also cc @Trott who commented saying that he preferred to close this but then removed the comment.

I have no opinion on this. I'm fine with it landing, and fine with closing it.

@lpinca lpinca force-pushed the enable/no-else-return branch from e404e2c to 9b40e55 Compare April 19, 2020 06:10
@lpinca lpinca removed the blocked PRs that are blocked by other issues or PRs. label Apr 19, 2020
@lpinca lpinca force-pushed the enable/no-else-return branch 2 times, most recently from e590a4f to 17c648a Compare April 25, 2020 17:55
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/31154/

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 3, 2020
@lpinca lpinca force-pushed the enable/no-else-return branch from 17c648a to 3c3c4e5 Compare May 12, 2020 18:00
@lpinca lpinca mentioned this pull request May 15, 2020
Closed
2 tasks
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/31345/

@nodejs nodejs deleted a comment from nodejs-github-bot May 15, 2020
@nodejs nodejs deleted a comment from nodejs-github-bot May 15, 2020
@lpinca lpinca force-pushed the enable/no-else-return branch from 3c3c4e5 to af70a1c Compare May 15, 2020 14:37
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/31349/

lpinca added a commit that referenced this pull request May 16, 2020
@lpinca
tools: enable no-else-return lint rule
b533fb3 
Refs: #32644
Refs: #32662

PR-URL: #32667
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@lpinca
Copy link
Member Author

lpinca commented May 16, 2020

Landed in b533fb3.

@lpinca lpinca closed this May 16, 2020
@lpinca lpinca deleted the enable/no-else-return branch May 16, 2020 04:44
codebytere pushed a commit that referenced this pull request May 16, 2020
@lpinca @codebytere
tools: enable no-else-return lint rule
9119344 
Refs: #32644
Refs: #32662

PR-URL: #32667
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@codebytere codebytere mentioned this pull request May 18, 2020
Merged
@codebytere
Copy link
Member

@lpinca would you mind backporting this to v12.x? there are a solid handful of conflicts.

@lpinca
Copy link
Member Author

lpinca commented Jul 9, 2020

yes, I will do it later today.

@lpinca lpinca mentioned this pull request Jul 9, 2020
Closed
4 tasks
lpinca added a commit to lpinca/node that referenced this pull request Jul 9, 2020
@lpinca
tools: enable no-else-return lint rule
441e6d9 
Refs: nodejs#32644
Refs: nodejs#32662

PR-URL: nodejs#32667
Backport-PR-URL: nodejs#34275
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
codebytere pushed a commit that referenced this pull request Jul 13, 2020
@lpinca @codebytere
tools: enable no-else-return lint rule
a131c72 
Refs: #32644
Refs: #32662

PR-URL: #32667
Backport-PR-URL: #34275
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@codebytere codebytere mentioned this pull request Jul 13, 2020
Merged
codebytere pushed a commit that referenced this pull request Jul 14, 2020
@lpinca @codebytere
tools: enable no-else-return lint rule
248f9af 
Refs: #32644
Refs: #32662

PR-URL: #32667
Backport-PR-URL: #34275
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trivikr trivikr trivikr approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@lpinca @targos @devsnek @addaleax @Trott @nodejs-github-bot @codebytere @BridgeAR @trivikr