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

* Enable 'prefer-switch' rule * Change default min-cases to 3
palantir · May 4, 2017 · 21db100 · 21db100
1 parent e1e1e9c
commit 21db100
Show file tree

Hide file tree

Showing 11 changed files with 127 additions and 101 deletions.
diff --git a/src/enableDisableRules.ts b/src/enableDisableRules.ts
@@ -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";

diff --git a/src/language/utils.ts b/src/language/utils.ts
@@ -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;
 }
 
 /** 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;
 }
 
 /**

diff --git a/src/rules/indentRule.ts b/src/rules/indentRule.ts
@@ -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)) {

diff --git a/src/rules/noUnnecessaryTypeAssertionRule.ts b/src/rules/noUnnecessaryTypeAssertionRule.ts
@@ -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);

diff --git a/src/rules/preferConstRule.ts b/src/rules/preferConstRule.ts
@@ -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;
         }
     }
 

diff --git a/src/rules/preferSwitchRule.ts b/src/rules/preferSwitchRule.ts
@@ -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: {
@@ -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];

diff --git a/test/rules/prefer-switch/default/test.ts.lint b/test/rules/prefer-switch/default/test.ts.lint
@@ -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.
diff --git a/test/rules/prefer-switch/min-cases-2/test.ts.lint b/test/rules/prefer-switch/min-cases-2/test.ts.lint
@@ -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.
diff --git a/...les/prefer-switch/min-cases-3/tslint.json → ...les/prefer-switch/min-cases-2/tslint.json b/...les/prefer-switch/min-cases-3/tslint.json → ...les/prefer-switch/min-cases-2/tslint.json
@@ -1,7 +1,7 @@
 {
   "rules": {
     "prefer-switch": [true, {
-        "min-cases": 3
+        "min-cases": 2
     }]
   }
 }
diff --git a/test/rules/prefer-switch/min-cases-3/test.ts.lint b/test/rules/prefer-switch/min-cases-3/test.ts.lint
diff --git a/tslint.json b/tslint.json
@@ -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,
@@ -82,7 +81,7 @@
       "public-before-private",
       "static-before-instance",
       "variables-before-functions"
-    ],    
+    ],
     "no-console": {
       "options": ["log"]
     },