Skip to content

tools: enable no-else-return ESLint rule #11159

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
Closed
wants to merge 2 commits into from
Closed

tools: enable no-else-return ESLint rule #11159

wants to merge 2 commits into from

Conversation

@Trott
Copy link
Member

@Trott Trott commented Feb 4, 2017

If an if block contains a return statement, the else block becomes unnecessary. Its contents can be placed outside of the block.

Refs: http://eslint.org/docs/rules/no-else-return

Inspired by #11148

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

tools test lib benchmark

@Trott Trott added benchmark Issues and PRs related to the benchmark subsystem. lib / src Issues and PRs related to general changes in the lib or src directory. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. labels Feb 4, 2017
@nodejs-github-bot nodejs-github-bot added dont-land-on-v4.x lib / src Issues and PRs related to general changes in the lib or src directory. tools Issues and PRs related to the tools directory. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Feb 4, 2017
mscdex
mscdex reviewed Feb 4, 2017
View reviewed changes
benchmark/_http-benchmarkers.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these blank lines should be removed.

joyeecheung
joyeecheung approved these changes Feb 4, 2017
View reviewed changes
Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

Rubber stamp-y LGTM except style nits

test/parallel/test-fs-utimes.js Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Indentation is off

test/parallel/test-buffer-fill.js Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Indentation is off

test/parallel/test-buffer-fill.js Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Indentation is off

lib/url.js Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Comment is deleted

lib/path.js Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Indentation is off

lib/fs.js Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Indentation is off

thefourtheye
thefourtheye reviewed Feb 4, 2017
View reviewed changes
Copy link
Contributor

@thefourtheye thefourtheye left a comment

Choose a reason for hiding this comment

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

I am not sure how the linter missed the indentation problems.

lib/_stream_readable.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation.

lib/path.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation.

@not-an-aardvark
Copy link
Contributor

I am not sure how the linter missed the indentation problems.

Currently, it does not check the indentation of comments. This is planned to change in the next major release.

cjihrig
cjihrig approved these changes Feb 4, 2017
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM with the extra empty lines removed.

@silverwind
Copy link
Contributor

I personally find it easier to visually parse code with the else branch intact, even if it's not strictly necessary.

mscdex
mscdex reviewed Feb 4, 2017
View reviewed changes
lib/url.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Still some blank lines like this.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's the only one, I think, and that one makes sense to me. Will remove it if anyone disagrees.

@joyeecheung
Copy link
Member

Also if we were using const or let in the else-branch before, be careful there can be TDZs...

@JacksonTian JacksonTian mentioned this pull request Feb 6, 2017
Closed
2 tasks
Trott added 2 commits February 19, 2017 08:05
@Trott
benchmark,lib,test,tools: remove else after return
cb916a4 
If there is a `return` in an `if` block, an `else` block is unnecessary.
It can simply be moved after the `if` block.

This is in preparation for a lint rule to enforce this practice.
@Trott
tools: enforce no else after return
078670e
@Trott
Copy link
Member Author

Trott commented Feb 21, 2017

I'm going to honor the preference expressed by @silverwind and @addaleax and close this. I don't feel strongly about it and it doesn't seem to come up as something in PR review comments. If someone else feels far more strongly about this than I do, feel free to re-open, comment, or open another PR.

@Trott Trott closed this Feb 21, 2017
@sam-github
Copy link
Contributor

I missed this until now, but belatedly, LGTM, I like the else-less style. It cuts down on indentation, and makes clear that once a conditional has been handled by return, code can continue indentless and read more linearly. But I don't feel really strong about it, I can read the code either way.

@Trott Trott deleted the no-else-return branch January 13, 2022 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@cjihrig cjihrig cjihrig approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

+3 more reviewers

@mscdex mscdex mscdex left review comments

@thefourtheye thefourtheye thefourtheye left review comments

@not-an-aardvark not-an-aardvark not-an-aardvark approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

benchmark Issues and PRs related to the benchmark subsystem. lib / src Issues and PRs related to general changes in the lib or src directory. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. whatwg-url Issues and PRs related to the WHATWG URL implementation.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@Trott @not-an-aardvark @silverwind @joyeecheung @sam-github @mscdex @jasnell @thefourtheye @cjihrig @nodejs-github-bot