Clean up preferFunctionOverMethodRule (palantir#2119)

More descriptive failure message, minor code style fixes
Arun-KumarH · Jan 26, 2017 · 6405fbf · 6405fbf
1 parent c2362e8
commit 6405fbf
Show file tree

Hide file tree

Showing 6 changed files with 30 additions and 22 deletions.
diff --git a/src/rules/preferFunctionOverMethodRule.ts b/src/rules/preferFunctionOverMethodRule.ts
@@ -26,10 +26,10 @@ export class Rule extends Lint.Rules.AbstractRule {
     /* tslint:disable:object-literal-sort-keys */
     public static metadata: Lint.IRuleMetadata = {
         ruleName: "prefer-function-over-method",
-        description: "Warns for methods that do not use 'this'.",
+        description: "Warns for class methods that do not use 'this'.",
         optionsDescription: Lint.Utils.dedent`
-            "${OPTION_ALLOW_PUBLIC}" excludes public methods.
-            "${OPTION_ALLOW_PROTECTED}" excludes protected methods.`,
+            "${OPTION_ALLOW_PUBLIC}" excludes checking of public methods.
+            "${OPTION_ALLOW_PROTECTED}" excludes checking of protected methods.`,
         options: {
             type: "string",
             enum: [OPTION_ALLOW_PUBLIC, OPTION_ALLOW_PROTECTED],
@@ -43,14 +43,14 @@ export class Rule extends Lint.Rules.AbstractRule {
     };
     /* tslint:enable:object-literal-sort-keys */
 
-    public static FAILURE_STRING = "Method does not use 'this'. Use a function instead.";
+    public static FAILURE_STRING = "Class method does not use 'this'. Use a function instead.";
 
     public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] {
-        return this.applyWithWalker(new Walker(sourceFile, this.getOptions()));
+        return this.applyWithWalker(new PreferFunctionOverMethodWalker(sourceFile, this.getOptions()));
     }
 }
 
-class Walker extends Lint.RuleWalker {
+class PreferFunctionOverMethodWalker extends Lint.RuleWalker {
     private allowPublic = this.hasOption(OPTION_ALLOW_PUBLIC);
     private allowProtected = this.hasOption(OPTION_ALLOW_PROTECTED);
     private stack: ThisUsed[] = [];
@@ -66,10 +66,11 @@ class Walker extends Lint.RuleWalker {
                 const { name } = node as ts.MethodDeclaration;
                 const usesThis = this.withThisScope(
                     name.kind === ts.SyntaxKind.Identifier ? name.text : undefined,
-                    () => super.visitNode(node));
-                if (!usesThis &&
-                    node.parent!.kind !== ts.SyntaxKind.ObjectLiteralExpression &&
-                    this.shouldWarnForModifiers(node as ts.MethodDeclaration)) {
+                    () => super.visitNode(node),
+                );
+                if (!usesThis
+                        && node.parent!.kind !== ts.SyntaxKind.ObjectLiteralExpression
+                        && this.shouldWarnForModifiers(node as ts.MethodDeclaration)) {
                     this.addFailureAtNode((node as ts.MethodDeclaration).name, Rule.FAILURE_STRING);
                 }
                 break;
@@ -114,19 +115,26 @@ class Walker extends Lint.RuleWalker {
     }
 }
 
-interface ThisUsed { readonly name: string | undefined; isThisUsed: boolean; }
+interface ThisUsed {
+    readonly name: string | undefined;
+    isThisUsed: boolean;
+}
 
 function isRecursiveCall(thisOrSuper: ts.Node, cur: ThisUsed) {
     const parent = thisOrSuper.parent!;
-    return thisOrSuper.kind === ts.SyntaxKind.ThisKeyword &&
-        parent.kind === ts.SyntaxKind.PropertyAccessExpression &&
-        (parent as ts.PropertyAccessExpression).name.text === cur.name;
+    return thisOrSuper.kind === ts.SyntaxKind.ThisKeyword
+        && parent.kind === ts.SyntaxKind.PropertyAccessExpression
+        && (parent as ts.PropertyAccessExpression).name.text === cur.name;
 }
 
 const enum Visibility { Public, Protected, Private }
 
 function methodVisibility(node: ts.MethodDeclaration): Visibility {
-    return Lint.hasModifier(node.modifiers, ts.SyntaxKind.PrivateKeyword) ? Visibility.Private :
-        Lint.hasModifier(node.modifiers, ts.SyntaxKind.ProtectedKeyword) ? Visibility.Protected :
-        Visibility.Public;
+    if (Lint.hasModifier(node.modifiers, ts.SyntaxKind.PrivateKeyword)) {
+        return Visibility.Private;
+    } else if (Lint.hasModifier(node.modifiers, ts.SyntaxKind.ProtectedKeyword)) {
+        return Visibility.Protected;
+    } else {
+        return Visibility.Public;
+    }
 }
diff --git a/test/rules/prefer-function-over-method/allow-protected/test.ts.lint b/test/rules/prefer-function-over-method/allow-protected/test.ts.lint
@@ -6,4 +6,4 @@ class C {
             ~ [0]
 }
 
-[0]: Method does not use 'this'. Use a function instead.
+[0]: Class method does not use 'this'. Use a function instead.
diff --git a/test/rules/prefer-function-over-method/allow-public-and-protected/test.ts.lint b/test/rules/prefer-function-over-method/allow-public-and-protected/test.ts.lint
@@ -5,4 +5,4 @@ class C {
             ~ [0]
 }
 
-[0]: Method does not use 'this'. Use a function instead.
+[0]: Class method does not use 'this'. Use a function instead.
diff --git a/test/rules/prefer-function-over-method/allow-public/test.ts.lint b/test/rules/prefer-function-over-method/allow-public/test.ts.lint
@@ -6,4 +6,4 @@ class C {
             ~ [0]
 }
 
-[0]: Method does not use 'this'. Use a function instead.
+[0]: Class method does not use 'this'. Use a function instead.
diff --git a/test/rules/prefer-function-over-method/default/test.js.lint b/test/rules/prefer-function-over-method/default/test.js.lint
@@ -1,4 +1,4 @@
 class C {
     a() {}
-    ~ [Method does not use 'this'. Use a function instead.]
+    ~ [Class method does not use 'this'. Use a function instead.]
 }
diff --git a/test/rules/prefer-function-over-method/default/test.ts.lint b/test/rules/prefer-function-over-method/default/test.ts.lint
@@ -48,4 +48,4 @@ const o = {
     x() {}
 }
 
-[0]: Method does not use 'this'. Use a function instead.
+[0]: Class method does not use 'this'. Use a function instead.
-Original file line number
+Diff line change
@@ Expand Up / @@ -6,4 +6,4 @@ class C { @@
                 ~ [0]
     }
-    [0]: Method does not use 'this'. Use a function instead.
+    [0]: Class method does not use 'this'. Use a function instead.