Added space-before-function-paren rule #1897

cameron-mcateer · 2016-12-18T06:15:32Z

PR checklist

Addresses an existing issue: #0000
New feature, bugfix, or enhancement
- Includes tests
Documentation update

What changes did you make?

I added a rule which lets you enforce whether a space should exist or not before the opening parenthesis of a named function, anonymous function, async arrow function, method, or constructor. This is meant to be similar to the same-named rule for eslint.

Is there anything you'd like reviewers to focus on?

Performance of rule evaluation (specifically the use of a ts.Scanner for checking spaces before parenthesis).
The completeness of the tests

palantirtech · 2016-12-18T06:15:32Z

Thanks for your interest in palantir/tslint, @cameron-mcateer! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

nchen63 · 2016-12-19T03:45:27Z

can you turn on circleCI?

cameron-mcateer · 2016-12-19T04:13:05Z

~~Sorry I am new to contributing to tslint and can't find anything anything enabling circleCI. Do you have a link?~~
@nchen63 circleCI complete

adidahiya · 2016-12-19T16:29:47Z

src/rules/spaceBeforeFunctionParenRule.ts

+import * as Lint from "../index";
+
+const ALWAYS_OR_NEVER = {
+  enum: ["always", "never"],


please use 4-space indentation in TS files like the rest of the project

adidahiya · 2016-12-19T16:30:29Z

src/rules/spaceBeforeFunctionParenRule.ts

+    super.visitMethodSignature(node);
+  }
+
+  private _visitFunction(node: ts.Node): void {


we don't use the underscore naming convention -- the private modifier is enough to denote encapsulation.

adidahiya · 2016-12-19T16:31:40Z

src/rules/spaceBeforeFunctionParenRule.ts

+      failure = this.createFailure(openParen.getStart(), 1, Rule.MISSING_WHITESPACE_ERROR, fix);
+    }
+
+    if (failure) {


please avoid implicit type coercion to boolean -- do an explicit undefined check instead

adidahiya · 2016-12-19T16:32:01Z

src/rules/spaceBeforeFunctionParenRule.ts

+  private _visitFunction(node: ts.Node): void {
+    const options = this.getOptions();
+
+    if (options === null || options.length === 0 || Object.keys(options).length === 0) {


this check looks overly defensive; I don't think any of your code will break if you remove this. it's better to be consistent with other rules in code style.

adidahiya · 2016-12-19T16:33:31Z

src/rules/spaceBeforeFunctionParenRule.ts

+  private getOption(option: string): string {
+    const allOptions = this.getOptions();
+    if (allOptions == null || allOptions.length === 0) {
+        return null;


please avoid using null explicitly; return undefined instead

adidahiya · 2016-12-19T16:33:55Z

src/rules/spaceBeforeFunctionParenRule.ts

+      }
+    });
+
+    if (!openParen || (isArrow && !isAsyncArrow)) {


no implicit boolean coercion

adidahiya · 2016-12-19T16:34:47Z

src/rules/spaceBeforeFunctionParenRule.ts

+    type: "style",
+    typescriptOnly: false,
+  };
+  public static INVALID_WHITESPACE_ERROR = "Should be no space before paren";


suggestion: "Spaces before function parens are disallowed"

adidahiya · 2016-12-19T16:35:32Z

src/rules/spaceBeforeFunctionParenRule.ts

+    typescriptOnly: false,
+  };
+  public static INVALID_WHITESPACE_ERROR = "Should be no space before paren";
+  public static MISSING_WHITESPACE_ERROR = "Should be a space before paren";


suggestion: "Missing whitespace before function parens"

cameron-mcateer · 2016-12-19T16:58:25Z

@adidahiya Thanks for the feedback! Changes made.

cameron-mcateer · 2016-12-19T20:54:57Z

Updated. Renamed file from test.js.int to test.js.lint.

nchen63 · 2016-12-20T00:41:12Z

src/rules/spaceBeforeFunctionParenRule.ts

+
+            * \`"anonymous"\` checks before the opening paren in anonymous functions
+            * \`"named"\` checks before the opening paren in named functions
+            * \`"asyncArrow"\` checks before the opening paren in async arrow functions,


don't need comma at the end here

nchen63 · 2016-12-20T00:57:27Z

src/rules/spaceBeforeFunctionParenRule.ts

+        super.visitMethodSignature(node);
+    }
+
+    private visitFunction(node: ts.Node): void {


can you break this up into smaller functions?

nchen63 · 2016-12-20T01:08:25Z

src/rules/spaceBeforeFunctionParenRule.ts

+export class Rule extends Lint.Rules.AbstractRule {
+    public static metadata: Lint.IRuleMetadata = {
+        description: "Require or disallow a space before function parenthesis",
+        optionExamples: [`[true, {"anonymous": "always", "named": "never", "asyncArrow": "always"}]`],


seems tedious to need to specify a value for each type of function. How about allowing something like [true, "always"] or even [true], which defaults to the same thing.

@nchen63

deferring to @nchen63 for final review

cameron-mcateer · 2016-12-20T08:49:37Z

@nchen63 Split visitFunction() into multiple functions and moved location of failure to the space character for "never" failures.

nchen63 · 2016-12-23T17:13:17Z

src/rules/spaceBeforeFunctionParenRule.ts

+            * \`"named"\` checks before the opening paren in named functions
+            * \`"asyncArrow"\` checks before the opening paren in async arrow functions
+            * \`"method"\` checks before the opening paren in class methods
+            *v\`"constructor"\` checks before the opening paren in class constructors


nchen63 · 2016-12-23T17:19:05Z

src/rules/spaceBeforeFunctionParenRule.ts

+    }
+
+    private getOption(optionName: string): string {
+        const allOptions = this.getOptions();


can you parse the options once in the constructor so that doing this check will be a quick lookup?

cameron-mcateer · 2016-12-23T18:39:37Z

@nchen63 Woops sorry that snuck in. Also caching the options now.

cameron-mcateer · 2016-12-23T19:03:53Z

Rebased on latest to keep tests passing.

nchen63 · 2016-12-23T20:54:32Z

src/rules/spaceBeforeFunctionParenRule.ts

+
+class FunctionWalker extends Lint.RuleWalker {
+    private scanner: ts.Scanner;
+    private cachedOptions: any;


here's how you can avoid using any for cachedOptions:

interface CachedOptions { anonymous?: boolean; asyncArrow?: boolean; } type optionType = "anonymous" | "asyncArrow"; let cachedOptions: CachedOptions = {}; // cache the options let optionArray: optionType[] = ["anonymous", "asyncArrow"] for (const option of optionArray) { cachedOptions[option] = true; }

Oh thank you. I kept using the string values in so that it allows the user to ignore certain rules (as opposed to forcing a failure if they are not consistent).

nchen63 · 2016-12-24T01:05:39Z

@cameron-mcateer thanks!

For future reference, it would be helpful if you didn't squash and force push your changes so I can see the diff between updates.

cameron-mcateer · 2016-12-24T01:25:23Z

@nchen63 Thanks! Will do. I'm used to amending all my PR commits. I'll keep that in mind from now on.

cameron-mcateer force-pushed the master branch from e98e041 to 444ff58 Compare December 18, 2016 06:53

adidahiya previously requested changes Dec 19, 2016

View reviewed changes

adidahiya assigned nchen63 Dec 19, 2016

cameron-mcateer force-pushed the master branch from 444ff58 to 936078a Compare December 19, 2016 16:55

cameron-mcateer force-pushed the master branch from 936078a to dec8fed Compare December 19, 2016 20:51

nchen63 suggested changes Dec 20, 2016

View reviewed changes

cameron-mcateer force-pushed the master branch 3 times, most recently from 6395bc7 to 665ab24 Compare December 20, 2016 08:45

nchen63 suggested changes Dec 23, 2016

View reviewed changes

cameron-mcateer force-pushed the master branch from 665ab24 to 4ce2f1d Compare December 23, 2016 18:38

cameron-mcateer force-pushed the master branch from 4ce2f1d to 5d6cf1c Compare December 23, 2016 19:00

nchen63 reviewed Dec 23, 2016

View reviewed changes

Added space-before-function-paren rule

b562822

cameron-mcateer force-pushed the master branch from 5d6cf1c to b562822 Compare December 23, 2016 21:40

nchen63 approved these changes Dec 24, 2016

View reviewed changes

nchen63 merged commit 60e55d6 into palantir:master Dec 24, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added space-before-function-paren rule #1897

Added space-before-function-paren rule #1897

cameron-mcateer commented Dec 18, 2016 •

edited

Loading

palantirtech commented Dec 18, 2016

nchen63 commented Dec 19, 2016

cameron-mcateer commented Dec 19, 2016 •

edited

Loading

adidahiya Dec 19, 2016

adidahiya Dec 19, 2016

adidahiya Dec 19, 2016

adidahiya Dec 19, 2016

adidahiya Dec 19, 2016

adidahiya Dec 19, 2016

adidahiya Dec 19, 2016

adidahiya Dec 19, 2016

cameron-mcateer commented Dec 19, 2016

cameron-mcateer commented Dec 19, 2016

nchen63 Dec 20, 2016

nchen63 Dec 20, 2016

nchen63 Dec 20, 2016

cameron-mcateer commented Dec 20, 2016

nchen63 Dec 23, 2016

nchen63 Dec 23, 2016

cameron-mcateer commented Dec 23, 2016

cameron-mcateer commented Dec 23, 2016

nchen63 Dec 23, 2016

cameron-mcateer Dec 23, 2016

nchen63 commented Dec 24, 2016

cameron-mcateer commented Dec 24, 2016

Added space-before-function-paren rule #1897

Added space-before-function-paren rule #1897

Conversation

cameron-mcateer commented Dec 18, 2016 • edited Loading

PR checklist

What changes did you make?

Is there anything you'd like reviewers to focus on?

palantirtech commented Dec 18, 2016

nchen63 commented Dec 19, 2016

cameron-mcateer commented Dec 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cameron-mcateer commented Dec 19, 2016

cameron-mcateer commented Dec 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cameron-mcateer commented Dec 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cameron-mcateer commented Dec 23, 2016

cameron-mcateer commented Dec 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nchen63 commented Dec 24, 2016

cameron-mcateer commented Dec 24, 2016

cameron-mcateer commented Dec 18, 2016 •

edited

Loading

cameron-mcateer commented Dec 19, 2016 •

edited

Loading