Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Enable 'prefer-switch' rule and raise default 'min-cases' #2669

Merged
merged 2 commits into from
May 4, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/enableDisableRules.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@
* limitations under the License.
*/

// tslint:disable prefer-switch
// (waiting on https://github.com/palantir/tslint/pull/2369)

import * as utils from "tsutils";
import * as ts from "typescript";
import { IOptions } from "./language/rule/rule";
Expand Down
78 changes: 46 additions & 32 deletions src/language/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -339,44 +339,58 @@ export function forEachComment(node: ts.Node, cb: ForEachCommentCallback) {

/** Exclude leading positions that would lead to scanning for trivia inside JsxText */
function canHaveLeadingTrivia(tokenKind: ts.SyntaxKind, parent: ts.Node): boolean {
if (tokenKind === ts.SyntaxKind.JsxText) {
return false; // there is no trivia before JsxText
}
if (tokenKind === ts.SyntaxKind.OpenBraceToken) {
// before a JsxExpression inside a JsxElement's body can only be other JsxChild, but no trivia
return parent.kind !== ts.SyntaxKind.JsxExpression || parent.parent!.kind !== ts.SyntaxKind.JsxElement;
}
if (tokenKind === ts.SyntaxKind.LessThanToken) {
if (parent.kind === ts.SyntaxKind.JsxClosingElement) {
return false; // would be inside the element body
}
if (parent.kind === ts.SyntaxKind.JsxOpeningElement || parent.kind === ts.SyntaxKind.JsxSelfClosingElement) {
// there can only be leading trivia if we are at the end of the top level element
return parent.parent!.parent!.kind !== ts.SyntaxKind.JsxElement;
}
switch (tokenKind) {
case ts.SyntaxKind.JsxText:
return false; // there is no trivia before JsxText

case ts.SyntaxKind.OpenBraceToken:
// before a JsxExpression inside a JsxElement's body can only be other JsxChild, but no trivia
return parent.kind !== ts.SyntaxKind.JsxExpression || parent.parent!.kind !== ts.SyntaxKind.JsxElement;

case ts.SyntaxKind.LessThanToken:
switch (parent.kind) {
case ts.SyntaxKind.JsxClosingElement:
return false; // would be inside the element body
case ts.SyntaxKind.JsxOpeningElement:
case ts.SyntaxKind.JsxSelfClosingElement:
// there can only be leading trivia if we are at the end of the top level element
return parent.parent!.parent!.kind !== ts.SyntaxKind.JsxElement;
default:
return true;
}

default:
return true;
}
return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

you could leave this return and remove the default clauses above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the default so it's easy to tell that there's no fallthrough.

}

/** Exclude trailing positions that would lead to scanning for trivia inside JsxText */
function canHaveTrailingTrivia(tokenKind: ts.SyntaxKind, parent: ts.Node): boolean {
if (tokenKind === ts.SyntaxKind.JsxText) {
return false; // there is no trivia after JsxText
}
if (tokenKind === ts.SyntaxKind.CloseBraceToken) {
// after a JsxExpression inside a JsxElement's body can only be other JsxChild, but no trivia
return parent.kind !== ts.SyntaxKind.JsxExpression || parent.parent!.kind !== ts.SyntaxKind.JsxElement;
}
if (tokenKind === ts.SyntaxKind.GreaterThanToken) {
if (parent.kind === ts.SyntaxKind.JsxOpeningElement) {
return false; // would be inside the element
}
if (parent.kind === ts.SyntaxKind.JsxClosingElement || parent.kind === ts.SyntaxKind.JsxSelfClosingElement) {
// there can only be trailing trivia if we are at the end of the top level element
return parent.parent!.parent!.kind !== ts.SyntaxKind.JsxElement;
}
switch (tokenKind) {
case ts.SyntaxKind.JsxText:
// there is no trivia after JsxText
return false;

case ts.SyntaxKind.CloseBraceToken:
// after a JsxExpression inside a JsxElement's body can only be other JsxChild, but no trivia
return parent.kind !== ts.SyntaxKind.JsxExpression || parent.parent!.kind !== ts.SyntaxKind.JsxElement;

case ts.SyntaxKind.GreaterThanToken:
switch (parent.kind) {
case ts.SyntaxKind.JsxOpeningElement:
return false; // would be inside the element
case ts.SyntaxKind.JsxClosingElement:
case ts.SyntaxKind.JsxSelfClosingElement:
// there can only be trailing trivia if we are at the end of the top level element
return parent.parent!.parent!.kind !== ts.SyntaxKind.JsxElement;

default:
return true;
}

default:
return true;
}
return true;
}

/**
Expand Down
11 changes: 6 additions & 5 deletions src/rules/indentRule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -128,11 +128,12 @@ class IndentWalker extends Lint.RuleWalker {
}
}

if (currentScannedType === ts.SyntaxKind.SingleLineCommentTrivia
|| currentScannedType === ts.SyntaxKind.MultiLineCommentTrivia
|| currentScannedType === ts.SyntaxKind.NewLineTrivia) {
// ignore lines that have comments before the first token
continue;
switch (currentScannedType) {
case ts.SyntaxKind.SingleLineCommentTrivia:
case ts.SyntaxKind.MultiLineCommentTrivia:
case ts.SyntaxKind.NewLineTrivia:
// ignore lines that have comments before the first token
continue;
}

if (fullLeadingWhitespace.match(this.regExp)) {
Expand Down
9 changes: 5 additions & 4 deletions src/rules/noUnnecessaryTypeAssertionRule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,11 @@ class Walker extends Lint.AbstractWalker<void> {

public walk(sourceFile: ts.SourceFile) {
const cb = (node: ts.Node): void => {
if (node.kind === ts.SyntaxKind.TypeAssertionExpression ||
node.kind === ts.SyntaxKind.NonNullExpression ||
node.kind === ts.SyntaxKind.AsExpression) {
this.verifyCast(node as ts.TypeAssertion | ts.NonNullExpression | ts.AsExpression);
switch (node.kind) {
case ts.SyntaxKind.TypeAssertionExpression:
case ts.SyntaxKind.NonNullExpression:
case ts.SyntaxKind.AsExpression:
this.verifyCast(node as ts.TypeAssertion | ts.NonNullExpression | ts.AsExpression);
}

return ts.forEachChild(node, cb);
Expand Down
56 changes: 32 additions & 24 deletions src/rules/preferConstRule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -201,33 +201,41 @@ class PreferConstWalker extends Lint.AbstractWalker<Options> {
}

private handleExpression(node: ts.Expression): void {
if (node.kind === ts.SyntaxKind.Identifier) {
this.scope.reassigned.add((node as ts.Identifier).text);
} else if (node.kind === ts.SyntaxKind.ParenthesizedExpression) {
return this.handleExpression((node as ts.ParenthesizedExpression).expression);
} else if (node.kind === ts.SyntaxKind.ArrayLiteralExpression) {
for (const element of (node as ts.ArrayLiteralExpression).elements) {
if (element.kind === ts.SyntaxKind.SpreadElement) {
this.handleExpression((element as ts.SpreadElement).expression);
} else {
this.handleExpression(element);
}
}
} else if (node.kind === ts.SyntaxKind.ObjectLiteralExpression) {
for (const property of (node as ts.ObjectLiteralExpression).properties) {
if (property.kind === ts.SyntaxKind.ShorthandPropertyAssignment) {
this.scope.reassigned.add(property.name.text);
} else if (property.kind === ts.SyntaxKind.SpreadAssignment) {
if (property.name !== undefined) {
this.scope.reassigned.add((property.name as ts.Identifier).text);
switch (node.kind) {
case ts.SyntaxKind.Identifier:
this.scope.reassigned.add((node as ts.Identifier).text);
break;
case ts.SyntaxKind.ParenthesizedExpression:
this.handleExpression((node as ts.ParenthesizedExpression).expression);
break;
case ts.SyntaxKind.ArrayLiteralExpression:
for (const element of (node as ts.ArrayLiteralExpression).elements) {
if (element.kind === ts.SyntaxKind.SpreadElement) {
this.handleExpression((element as ts.SpreadElement).expression);
} else {
// handle `...(variable)`
this.handleExpression(property.expression!);
this.handleExpression(element);
}
} else {
this.handleExpression((property as ts.PropertyAssignment).initializer);
}
}
break;
case ts.SyntaxKind.ObjectLiteralExpression:
for (const property of (node as ts.ObjectLiteralExpression).properties) {
switch (property.kind) {
case ts.SyntaxKind.ShorthandPropertyAssignment:
this.scope.reassigned.add(property.name.text);
break;
case ts.SyntaxKind.SpreadAssignment:
if (property.name !== undefined) {
this.scope.reassigned.add((property.name as ts.Identifier).text);
} else {
// handle `...(variable)`
this.handleExpression(property.expression!);
}
break;
default:
this.handleExpression((property as ts.PropertyAssignment).initializer);
}
}
break;
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/rules/preferSwitchRule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ export class Rule extends Lint.Rules.AbstractRule {
optionsDescription: Lint.Utils.dedent`
An optional object with the property '${OPTION_MIN_CASES}'.
This is the number cases needed before a switch statement is recommended.
Defaults to 2.`,
Defaults to 3.`,
options: {
type: "object",
properties: {
Expand All @@ -46,7 +46,7 @@ export class Rule extends Lint.Rules.AbstractRule {
public static FAILURE_STRING = "Use a switch statement instead of using multiple '===' checks.";

public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] {
let minCases = 2;
let minCases = 3;
if (this.ruleArguments.length) {
const obj = this.ruleArguments[0] as { "min-cases": number };
minCases = obj[OPTION_MIN_CASES];
Expand Down
28 changes: 3 additions & 25 deletions test/rules/prefer-switch/default/test.ts.lint
Original file line number Diff line number Diff line change
@@ -1,28 +1,6 @@
if (x === 1) {}
~~~~~~~ [0]
else if (x === 2) {} // Notice that we don't get *another* error here.
// Dangling `else` OK (this becomes `default:`)
else if (x === 3) {}
else {}
if (x === 1 || x === 2) {}

if (x === 1) {} else if (x === 2) {}
~~~~~~~ [0]

// Works with `||`
if (this === 1 || this === 2) {}
~~~~~~~~~~~~~~~~~~~~~~~~ [0]

// Default minumum of 2 cases.
if (x === 1) {} else {}

// Different variables, no failure
if (x === 1) {} else if (y === 2) {}

// Non-simple cases, no failure
if (x === f()) {} else if (x === g()) {}

// Allow properties
if (x.y.z === a.b) else if (x.y.z === c.d) {}
~~~~~~~~~~~~~ [0]
if (x === 1 || x === 2 || x === 3) {}
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [0]

[0]: Use a switch statement instead of using multiple '===' checks.
28 changes: 28 additions & 0 deletions test/rules/prefer-switch/min-cases-2/test.ts.lint
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
if (x === 1) {}
~~~~~~~ [0]
else if (x === 2) {} // Notice that we don't get *another* error here.
// Dangling `else` OK (this becomes `default:`)
else if (x === 3) {}
else {}

if (x === 1) {} else if (x === 2) {}
~~~~~~~ [0]

// Works with `||`
if (this === 1 || this === 2) {}
~~~~~~~~~~~~~~~~~~~~~~~~ [0]

// Default minumum of 2 cases.
if (x === 1) {} else {}

// Different variables, no failure
if (x === 1) {} else if (y === 2) {}

// Non-simple cases, no failure
if (x === f()) {} else if (x === g()) {}

// Allow properties
if (x.y.z === a.b) else if (x.y.z === c.d) {}
~~~~~~~~~~~~~ [0]

[0]: Use a switch statement instead of using multiple '===' checks.
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"rules": {
"prefer-switch": [true, {
"min-cases": 3
"min-cases": 2
}]
}
}
6 changes: 0 additions & 6 deletions test/rules/prefer-switch/min-cases-3/test.ts.lint

This file was deleted.

3 changes: 1 addition & 2 deletions tslint.json
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@
"no-use-before-declare": false,
"no-void-expression": false,
"prefer-function-over-method": false,
"prefer-switch": false,
"prefer-template": false,
"restrict-plus-operands": false,
"return-undefined": false,
Expand All @@ -82,7 +81,7 @@
"public-before-private",
"static-before-instance",
"variables-before-functions"
],
],
"no-console": {
"options": ["log"]
},
Expand Down