no-magic-numbers: Make condition lazy (don't call .getText() for ev…

…ery number literal in the file) (palantir#2042)
mxl · Jan 15, 2017 · da603e6 · da603e6
1 parent 96b8424
commit da603e6
Show file tree

Hide file tree

Showing 4 changed files with 25 additions and 17 deletions.
diff --git a/docs/_data/rules.json b/docs/_data/rules.json
@@ -1720,7 +1720,7 @@
     "ruleName": "whitespace",
     "description": "Enforces whitespace style conventions.",
     "rationale": "Helps maintain a readable, consistent style in your codebase.",
-    "optionsDescription": "\nSeven arguments may be optionally provided:\n\n* `\"check-branch\"` checks branching statements (`if`/`else`/`for`/`while`) are followed by whitespace.\n* `\"check-decl\"`checks that variable declarations have whitespace around the equals token.\n* `\"check-operator\"` checks for whitespace around operator tokens.\n* `\"check-module\"` checks for whitespace in import & export statements.\n* `\"check-separator\"` checks for whitespace after separator tokens (`,`/`;`).\n* `\"check-type\"` checks for whitespace before a variable type specification.\n* `\"check-typecast\"` checks for whitespace between a typecast and its target.",
+    "optionsDescription": "\nSeven arguments may be optionally provided:\n\n* `\"check-branch\"` checks branching statements (`if`/`else`/`for`/`while`) are followed by whitespace.\n* `\"check-decl\"`checks that variable declarations have whitespace around the equals token.\n* `\"check-operator\"` checks for whitespace around operator tokens.\n* `\"check-module\"` checks for whitespace in import & export statements.\n* `\"check-separator\"` checks for whitespace after separator tokens (`,`/`;`).\n* `\"check-type\"` checks for whitespace before a variable type specification.\n* `\"check-typecast\"` checks for whitespace between a typecast and its target.\n* `\"check-preblock\"` checks for whitespace before the opening brace of a block",
     "options": {
       "type": "array",
       "items": {
@@ -1732,7 +1732,8 @@
           "check-module",
           "check-separator",
           "check-type",
-          "check-typecast"
+          "check-typecast",
+          "check-preblock"
         ]
       },
       "minLength": 0,

diff --git a/docs/rules/whitespace/index.html b/docs/rules/whitespace/index.html
@@ -13,6 +13,7 @@
   * `"check-separator"` checks for whitespace after separator tokens (`,`/`;`).
   * `"check-type"` checks for whitespace before a variable type specification.
   * `"check-typecast"` checks for whitespace between a typecast and its target.
+  * `"check-preblock"` checks for whitespace before the opening brace of a block
 options:
   type: array
   items:
@@ -25,6 +26,7 @@
       - check-separator
       - check-type
       - check-typecast
+      - check-preblock
   minLength: 0
   maxLength: 7
 optionExamples:
@@ -45,7 +47,8 @@
         "check-module",
         "check-separator",
         "check-type",
-        "check-typecast"
+        "check-typecast",
+        "check-preblock"
       ]
     },
     "minLength": 0,

diff --git a/src/rules/noMagicNumbersRule.ts b/src/rules/noMagicNumbersRule.ts
@@ -68,7 +68,7 @@ export class Rule extends Lint.Rules.AbstractRule {
 }
 
 class NoMagicNumbersWalker extends Lint.RuleWalker {
-    // lookup object for allowed magic numbers
+    // allowed magic numbers
     private allowed: Set<string>;
     constructor(sourceFile: ts.SourceFile, options: IOptions) {
         super(sourceFile, options);
@@ -79,25 +79,28 @@ class NoMagicNumbersWalker extends Lint.RuleWalker {
     }
 
     public visitNode(node: ts.Node) {
-        if (node.kind === ts.SyntaxKind.NumericLiteral || this.isNegativeNumericExpression(node)) {
-            const isAllowedNode = Rule.ALLOWED_NODES.has(node.parent!.kind);
-            const isAllowedNumber = this.allowed.has(node.getText());
-            if (!isAllowedNode && !isAllowedNumber) {
+        const num = getLiteralNumber(node);
+        if (num !== undefined) {
+            if (!Rule.ALLOWED_NODES.has(node.parent!.kind) && !this.allowed.has(num)) {
                 this.addFailureAtNode(node, Rule.FAILURE_STRING);
             }
         } else {
             super.visitNode(node);
         }
     }
+}
 
-    /**
-     * Checks if a node is an negative unary expression with on a numeric operand.
-     */
-    private isNegativeNumericExpression(node: ts.Node): boolean {
-        if (node.kind !== ts.SyntaxKind.PrefixUnaryExpression) {
-            return false;
-        }
-        const unaryNode = (node as ts.PrefixUnaryExpression);
-        return unaryNode.operator === ts.SyntaxKind.MinusToken && unaryNode.operand.kind === ts.SyntaxKind.NumericLiteral;
+/** If node is a number literal, return a string representation of that number. */
+function getLiteralNumber(node: ts.Node): string | undefined {
+    if (node.kind === ts.SyntaxKind.NumericLiteral) {
+        return (node as ts.NumericLiteral).text;
+    }
+    if (node.kind !== ts.SyntaxKind.PrefixUnaryExpression) {
+        return undefined;
+    }
+    const { operator, operand } = node as ts.PrefixUnaryExpression;
+    if (operator === ts.SyntaxKind.MinusToken && operand.kind === ts.SyntaxKind.NumericLiteral) {
+        return "-" + (operand as ts.NumericLiteral).text;
     }
+    return undefined;
 }
diff --git a/test/rules/no-magic-numbers/custom/test.ts.lint b/test/rules/no-magic-numbers/custom/test.ts.lint
@@ -1,5 +1,6 @@
 console.log(1337);
 console.log(-1337);
+console.log(- 1337);
 console.log(1337.7);
 console.log(1338);
             ~~~~                      ['magic numbers' are not allowed]