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

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

  }
+  return result.requests.average;
+


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

  }
+    // ensure mtime has been written to disk


Indentation is off

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

test/parallel/test-buffer-fill.js

        }
+        elseWasLast = true;
+          // Once buffer.js calls the C++ implemenation of fill, return -1


Indentation is off

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

test/parallel/test-buffer-fill.js

        }
+        elseWasLast = true;
+          // Once buffer.js calls the C++ implemenation of fill, return -1


Indentation is off

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

lib/url.js

@@ -533,8 +533,7 @@ function autoEscapeStr(rest) {
  // There are ordinary characters at the end.
  if (lastEscapedPos < rest.length)
    return escaped + rest.slice(lastEscapedPos);
-  else  // The last character is escaped.
-    return escaped;
+  return escaped;


Comment is deleted

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

lib/path.js

+    }
+    for (i = path.length - 1; i >= start; --i) {
+      const code = path.charCodeAt(i);
+      if (code === 47/*/*/ || code === 92/*\*/) {
          // If we reached a path separator that was not part of a set of path


Indentation is off

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

lib/fs.js

  }
+    // Windows symlinks don't tolerate forward slashes.


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

@@ -18,18 +18,18 @@ function prependListener(emitter, event, fn) {
  // event emitter implementation with them.
  if (typeof emitter.prependListener === 'function') {
    return emitter.prependListener(event, fn);
-  } else {
+  }
    // This is a hack to make sure that our error handler is attached before any


Indentation.

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

lib/path.js

-        if (path.charCodeAt(i) === 47/*/*/) {
+    }
+    for (i = path.length - 1; i >= 0; --i) {
+      if (path.charCodeAt(i) === 47/*/*/) {
          // If we reached a path separator that was not part of a set of path


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

@@ -533,8 +533,9 @@ function autoEscapeStr(rest) {
  // There are ordinary characters at the end.
  if (lastEscapedPos < rest.length)
    return escaped + rest.slice(lastEscapedPos);
-  else  // The last character is escaped.
-    return escaped;
+


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

}

return result.requests.average;

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Trott commented Feb 4, 2017

mscdex Feb 4, 2017

joyeecheung left a comment

joyeecheung Feb 4, 2017

joyeecheung Feb 4, 2017

joyeecheung Feb 4, 2017

joyeecheung Feb 4, 2017

joyeecheung Feb 4, 2017

joyeecheung Feb 4, 2017

thefourtheye left a comment

thefourtheye Feb 4, 2017

thefourtheye Feb 4, 2017

not-an-aardvark commented Feb 4, 2017

cjihrig left a comment

silverwind commented Feb 4, 2017

mscdex Feb 4, 2017

Trott Feb 4, 2017

joyeecheung commented Feb 5, 2017

Trott commented Feb 21, 2017

sam-github commented Feb 22, 2017

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

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

Conversation

Trott commented Feb 4, 2017

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

joyeecheung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thefourtheye left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

not-an-aardvark commented Feb 4, 2017

cjihrig left a comment

Choose a reason for hiding this comment

silverwind commented Feb 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung commented Feb 5, 2017

Trott commented Feb 21, 2017

sam-github commented Feb 22, 2017