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.

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

Update: add allowElseIf option to no-else-return (fixes #9228) #9229

Merged
merged 8 commits into from Oct 2, 2017
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