tools: enable no-else-return ESLint rule by Trott · Pull Request #11159 · nodejs/node

Trott · 2017-02-04T05:13:13Z

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

mscdex · 2017-02-04T06:00:18Z

benchmark/_http-benchmarkers.js

I think these blank lines should be removed.

joyeecheung

Rubber stamp-y LGTM except style nits

joyeecheung · 2017-02-04T10:24:50Z

test/parallel/test-fs-utimes.js

Indentation is off

joyeecheung · 2017-02-04T10:25:14Z

test/parallel/test-buffer-fill.js

Indentation is off

joyeecheung · 2017-02-04T10:25:18Z

test/parallel/test-buffer-fill.js

Indentation is off

joyeecheung · 2017-02-04T10:25:47Z

lib/url.js

Comment is deleted

joyeecheung · 2017-02-04T10:26:19Z

lib/path.js

Indentation is off

joyeecheung · 2017-02-04T10:26:42Z

lib/fs.js

Indentation is off

thefourtheye

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

thefourtheye · 2017-02-04T11:39:12Z

lib/_stream_readable.js

Indentation.

thefourtheye · 2017-02-04T11:40:17Z

lib/path.js

Indentation.

not-an-aardvark · 2017-02-04T17:46:06Z

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

LGTM with the extra empty lines removed.

silverwind · 2017-02-04T21:15:28Z

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

mscdex · 2017-02-04T22:34:30Z

lib/url.js

Still some blank lines like this.

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

joyeecheung · 2017-02-05T09:43:20Z

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

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 · 2017-02-21T22:28:13Z

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.

sam-github · 2017-02-22T18:08:24Z

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 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 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 reviewed Feb 4, 2017

View reviewed changes

benchmark/_http-benchmarkers.js Outdated

Copy link

Contributor

mscdex Feb 4, 2017

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.

not-an-aardvark approved these changes Feb 4, 2017

View reviewed changes

joyeecheung approved these changes Feb 4, 2017

View reviewed changes

thefourtheye reviewed Feb 4, 2017

View reviewed changes

jasnell approved these changes Feb 4, 2017

View reviewed changes

cjihrig approved these changes Feb 4, 2017

View reviewed changes

Trott force-pushed the no-else-return branch from 0ccb16b to 1735e53 Compare February 4, 2017 21:28

mscdex reviewed Feb 4, 2017

View reviewed changes

JacksonTian mentioned this pull request Feb 6, 2017

child_process: charify the usage of 'else' #11148

Closed

2 tasks

Trott added 2 commits February 19, 2017 08:05

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.

tools: enforce no else after return

078670e

Trott force-pushed the no-else-return branch from 1735e53 to 078670e Compare February 19, 2017 16:05

Trott closed this Feb 21, 2017

Trott deleted the no-else-return branch January 13, 2022 22:34

Uh oh!

Comments

Conversation

Trott commented Feb 4, 2017

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheung left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thefourtheye left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

not-an-aardvark commented Feb 4, 2017

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

silverwind commented Feb 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Feb 5, 2017

Uh oh!

Trott commented Feb 21, 2017

Uh oh!

sam-github commented Feb 22, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants