Skip to content

Commit

Permalink
Update: add allowElseIf option to no-else-return (fixes #9228) (#9229)
Browse files Browse the repository at this point in the history
  • Loading branch information
graingert authored and not-an-aardvark committed Oct 2, 2017
1 parent 4567ab1 commit 1167638
Show file tree
Hide file tree
Showing 18 changed files with 196 additions and 31 deletions.
67 changes: 67 additions & 0 deletions docs/rules/no-else-return.md
Expand Up @@ -16,6 +16,23 @@ function foo() {

This rule is aimed at highlighting an unnecessary block of code following an `if` containing a return statement. As such, it will warn when it encounters an `else` following a chain of `if`s, all of them containing a `return` statement.

## Options

This rule has an object option:

```json
{
"no-else-return": ["error", { "allowElseIf": true }],
// or
"no-else-return": ["error", { "allowElseIf": false }]
}
```

* `allowElseIf: true` (default) allows `else if` blocks after a return
* `allowElseIf: false` disallows `else if` blocks after a return

### `allowElseIf: true`

Examples of **incorrect** code for this rule:

```js
Expand Down Expand Up @@ -49,6 +66,16 @@ function foo() {
return t;
}

function foo() {
if (error) {
return 'It failed';
} else {
if (loading) {
return "It's still loading";
}
}
}

// Two warnings for nested occurrences
function foo() {
if (x) {
Expand Down Expand Up @@ -95,4 +122,44 @@ function foo() {
return z;
}
}

function foo() {
if (error) {
return 'It failed';
} else if (loading) {
return "It's still loading";
}
}
```

### `allowElseIf: false`

Examples of **incorrect** code for this rule:

```js
/*eslint no-else-return: ["error", {allowElseIf: false}]*/

function foo() {
if (error) {
return 'It failed';
} else if (loading) {
return "It's still loading";
}
}
```

Examples of **correct** code for this rule:

```js
/*eslint no-else-return: ["error", {allowElseIf: false}]*/

function foo() {
if (error) {
return 'It failed';
}

if (loading) {
return "It's still loading";
}
}
```
3 changes: 2 additions & 1 deletion lib/ast-utils.js
Expand Up @@ -1041,7 +1041,8 @@ module.exports = {
} else if (parent.type === "Property" || parent.type === "MethodDefinition") {
if (parent.kind === "constructor") {
return "constructor";
} else if (parent.kind === "get") {
}
if (parent.kind === "get") {
tokens.push("getter");
} else if (parent.kind === "set") {
tokens.push("setter");
Expand Down
3 changes: 2 additions & 1 deletion lib/cli.js
Expand Up @@ -152,7 +152,8 @@ const cli = {
if (files.length) {
log.error("The --print-config option must be used with exactly one file name.");
return 1;
} else if (text) {
}
if (text) {
log.error("The --print-config option is not available for piped-in code.");
return 1;
}
Expand Down
3 changes: 2 additions & 1 deletion lib/formatters/html.js
Expand Up @@ -51,7 +51,8 @@ function renderSummary(totalErrors, totalWarnings) {
function renderColor(totalErrors, totalWarnings) {
if (totalErrors !== 0) {
return 2;
} else if (totalWarnings !== 0) {
}
if (totalWarnings !== 0) {
return 1;
}
return 0;
Expand Down
3 changes: 2 additions & 1 deletion lib/rules/callback-return.js
Expand Up @@ -60,7 +60,8 @@ module.exports = {
if (node.type === "MemberExpression") {
if (node.object.type === "Identifier") {
return true;
} else if (node.object.type === "MemberExpression") {
}
if (node.object.type === "MemberExpression") {
return containsOnlyIdentifiers(node.object);
}
}
Expand Down
3 changes: 2 additions & 1 deletion lib/rules/capitalized-comments.js
Expand Up @@ -247,7 +247,8 @@ module.exports = {

if (capitalize === "always" && isLowercase) {
return false;
} else if (capitalize === "never" && isUppercase) {
}
if (capitalize === "never" && isUppercase) {
return false;
}

Expand Down
3 changes: 2 additions & 1 deletion lib/rules/new-cap.js
Expand Up @@ -178,7 +178,8 @@ module.exports = {

// char has no uppercase variant, so it's non-alphabetic
return "non-alpha";
} else if (firstChar === firstCharLower) {
}
if (firstChar === firstCharLower) {
return "lower";
}
return "upper";
Expand Down
6 changes: 4 additions & 2 deletions lib/rules/newline-before-return.js
Expand Up @@ -59,9 +59,11 @@ module.exports = {

if (parentType === "IfStatement") {
return isPrecededByTokens(node, ["else", ")"]);
} else if (parentType === "DoWhileStatement") {
}
if (parentType === "DoWhileStatement") {
return isPrecededByTokens(node, ["do"]);
} else if (parentType === "SwitchCase") {
}
if (parentType === "SwitchCase") {
return isPrecededByTokens(node, [":"]);
}
return isPrecededByTokens(node, [")"]);
Expand Down
3 changes: 2 additions & 1 deletion lib/rules/no-alert.js
Expand Up @@ -71,7 +71,8 @@ function isShadowed(scope, node) {
function isGlobalThisReferenceOrGlobalWindow(scope, node) {
if (scope.type === "global" && node.type === "ThisExpression") {
return true;
} else if (node.name === "window") {
}
if (node.name === "window") {
return !isShadowed(scope, node);
}

Expand Down
3 changes: 2 additions & 1 deletion lib/rules/no-control-regex.js
Expand Up @@ -31,7 +31,8 @@ module.exports = {
function getRegExp(node) {
if (node.value instanceof RegExp) {
return node.value;
} else if (typeof node.value === "string") {
}
if (typeof node.value === "string") {

const parent = context.getAncestors().pop();

Expand Down
51 changes: 43 additions & 8 deletions lib/rules/no-else-return.js
Expand Up @@ -24,8 +24,15 @@ module.exports = {
recommended: false
},

schema: [],

schema: [{
type: "object",
properties: {
allowElseIf: {
type: "boolean"
}
},
additionalProperties: false
}],
fixable: "code"
},

Expand Down Expand Up @@ -134,13 +141,13 @@ module.exports = {

/**
* Check to see if the node is valid for evaluation,
* meaning it has an else and not an else-if
* meaning it has an else.
*
* @param {Node} node The node being evaluated
* @returns {boolean} True if the node is valid
*/
function hasElse(node) {
return node.alternate && node.consequent && node.alternate.type !== "IfStatement";
return node.alternate && node.consequent;
}

/**
Expand Down Expand Up @@ -189,14 +196,15 @@ module.exports = {
return checkForReturnOrIf(node);
}


/**
* Check the if statement
* Check the if statement, but don't catch else-if blocks.
* @returns {void}
* @param {Node} node The node for the if statement to check
* @private
*/
function IfStatement(node) {
const parent = context.getAncestors().pop();
function checkIfWithoutElse(node) {
const parent = node.parent;
let consequents,
alternate;

Expand All @@ -221,13 +229,40 @@ module.exports = {
}
}

/**
* Check the if statement
* @returns {void}
* @param {Node} node The node for the if statement to check
* @private
*/
function checkIfWithElse(node) {
const parent = node.parent;


/*
* Fixing this would require splitting one statement into two, so no error should
* be reported if this node is in a position where only one statement is allowed.
*/
if (!astUtils.STATEMENT_LIST_PARENTS.has(parent.type)) {
return;
}

const alternate = node.alternate;

if (alternate && alwaysReturns(node.consequent)) {
displayReport(alternate);
}
}

const allowElseIf = !(context.options[0] && context.options[0].allowElseIf === false);

//--------------------------------------------------------------------------
// Public API
//--------------------------------------------------------------------------

return {

"IfStatement:exit": IfStatement
"IfStatement:exit": allowElseIf ? checkIfWithoutElse : checkIfWithElse

};

Expand Down
9 changes: 6 additions & 3 deletions lib/rules/no-extra-parens.js
Expand Up @@ -195,10 +195,12 @@ module.exports = {
function containsAssignment(node) {
if (node.type === "AssignmentExpression") {
return true;
} else if (node.type === "ConditionalExpression" &&
}
if (node.type === "ConditionalExpression" &&
(node.consequent.type === "AssignmentExpression" || node.alternate.type === "AssignmentExpression")) {
return true;
} else if ((node.left && node.left.type === "AssignmentExpression") ||
}
if ((node.left && node.left.type === "AssignmentExpression") ||
(node.right && node.right.type === "AssignmentExpression")) {
return true;
}
Expand All @@ -219,7 +221,8 @@ module.exports = {

if (node.type === "ReturnStatement") {
return node.argument && containsAssignment(node.argument);
} else if (node.type === "ArrowFunctionExpression" && node.body.type !== "BlockStatement") {
}
if (node.type === "ArrowFunctionExpression" && node.body.type !== "BlockStatement") {
return containsAssignment(node.body);
}
return containsAssignment(node);
Expand Down
12 changes: 8 additions & 4 deletions lib/rules/no-mixed-requires.js
Expand Up @@ -104,14 +104,16 @@ module.exports = {

// "var x = require('util');"
return DECL_REQUIRE;
} else if (allowCall &&
}
if (allowCall &&
initExpression.type === "CallExpression" &&
initExpression.callee.type === "CallExpression"
) {

// "var x = require('diagnose')('sub-module');"
return getDeclarationType(initExpression.callee);
} else if (initExpression.type === "MemberExpression") {
}
if (initExpression.type === "MemberExpression") {

// "var x = require('glob').Glob;"
return getDeclarationType(initExpression.object);
Expand All @@ -131,7 +133,8 @@ module.exports = {

// "var x = require('glob').Glob;"
return inferModuleType(initExpression.object);
} else if (initExpression.arguments.length === 0) {
}
if (initExpression.arguments.length === 0) {

// "var x = require();"
return REQ_COMPUTED;
Expand All @@ -149,7 +152,8 @@ module.exports = {

// "var fs = require('fs');"
return REQ_CORE;
} else if (/^\.{0,2}\//.test(arg.value)) {
}
if (/^\.{0,2}\//.test(arg.value)) {

// "var utils = require('./utils');"
return REQ_FILE;
Expand Down
6 changes: 4 additions & 2 deletions lib/rules/sort-imports.js
Expand Up @@ -67,9 +67,11 @@ module.exports = {
function usedMemberSyntax(node) {
if (node.specifiers.length === 0) {
return "none";
} else if (node.specifiers[0].type === "ImportNamespaceSpecifier") {
}
if (node.specifiers[0].type === "ImportNamespaceSpecifier") {
return "all";
} else if (node.specifiers.length === 1) {
}
if (node.specifiers.length === 1) {
return "single";
}
return "multiple";
Expand Down
3 changes: 2 additions & 1 deletion lib/util/source-code.js
Expand Up @@ -332,7 +332,8 @@ class SourceCode extends TokenStore {
}

return parent && parent.type !== "FunctionDeclaration" && parent.type !== "Program" ? findJSDocComment(parentLeadingComments, parent.loc.start.line) : null;
} else if (leadingComments.length) {
}
if (leadingComments.length) {
return findJSDocComment(leadingComments, node.loc.start.line);
}

Expand Down
2 changes: 1 addition & 1 deletion packages/eslint-config-eslint/default.yml
Expand Up @@ -51,7 +51,7 @@ rules:
no-confusing-arrow: "error"
no-console: "error"
no-delete-var: "error"
no-else-return: "error"
no-else-return: ["error", { allowElseIf: false }]
no-eval: "error"
no-extend-native: "error"
no-extra-bind: "error"
Expand Down
3 changes: 2 additions & 1 deletion tests/lib/linter.js
Expand Up @@ -20,7 +20,8 @@
function compatRequire(name, windowName) {
if (typeof window === "object") {
return window[windowName || name];
} else if (typeof require === "function") {
}
if (typeof require === "function") {
return require(name);
}
throw new Error(`Cannot find object '${name}'.`);
Expand Down

0 comments on commit 1167638

Please sign in to comment.