Adds new rule "space-within-parens" #2959

aminpaks · 2017-06-26T20:32:37Z

PR checklist

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

Overview of change:

Adds new rule to verify amount of whitespace within parentheses.

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

Everything.

CHANGELOG.md entry:

[new-rule] space-within-parens

palantirtech · 2017-06-26T20:32:38Z

Thanks for your interest in palantir/tslint, @aminpaks! 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.

ajafff

I left some suggestions to improve the algorithm.

Regarding failing CI: in addition to running yarn test on your local machine you should also run yarn lint and fix the reported failures. If you want all in one, you can use yarn verify.

ajafff · 2017-06-27T08:09:48Z

src/rules/spaceWithinParensRule.ts

@@ -0,0 +1,167 @@
+/**
+ * @license
+ * Copyright 2013 Palantir Technologies, Inc.


ajafff · 2017-06-27T08:16:25Z

src/rules/spaceWithinParensRule.ts

+    }
+
+    private checkAndAddFailureAt(position: number, length: number, spaceCount: number = 0) {
+        if (this.failures.some(f => f.getStartPosition().getPosition() === position)) {


do you really need to remove duplicate failures here? If yes, there's probably something wrong with the rule

With your algorithm how should I know that if this is not the case below? Both open and close paren tokens will add a failure. this is the only place that we need to check.

console.log( ); ~ [open token] ~ [close token]

in my sample implementation there was a check if ch is a line break to avoid failures.

When checking the CloseBrace, you could add a similar check if ch === 40 to not fail if you reached the OpenBraceToken.

ajafff · 2017-06-27T08:17:41Z

src/rules/spaceWithinParensRule.ts

+    }
+
+    private getOpenParenToken(node: ts.Node): ts.Node | undefined {
+        return Lint.childOfKind(node, ts.SyntaxKind.OpenParenToken);


if you do this for every node, you could just use forEachToken from tsutils to visit every token instead of visiting every node and finding the paren tokens.

forEachToken doesn't exist in tsutils v2.4.0.
I tried import { forEachToken } from 'tsutils'; no luck.

are you sure? I just tried it (v2.4.0) and it works for me.
Maybe it's another error, e.g. unused variable.

Ah! it's utils not tsutils.

forEachToken has this signature (fullText: string, kind: ts.SyntaxKind, pos: TokenPosition) I'm using not using fullText and Typescript is complaining. What should I do?

Well, I really meant import {forEachToken} from "tsutils". But the one in ../utils is also ok.

Re unused parameter: either use fullText instead of this.sourceFile.textor rename the parameter to_fullText` (not the leading underscore)

ajafff · 2017-06-27T08:23:20Z

test/rules/space-within-parens/force-space/test.ts.lint

@@ -0,0 +1,34 @@
+// simple expression
+var x = [{


add a test for multiline call expression. This should pass IMO

foo( bar, baz, );

ajafff · 2017-06-27T08:26:15Z

src/rules/spaceWithinParensRule.ts

+        optionsDescription: Lint.Utils.dedent`
+            You may enforce the amount of whitespace within parentheses.
+        `,
+        options: {


simplify to

options: {type: "number", min: 0}

ajafff · 2017-06-27T08:31:54Z

test/rules/space-within-parens/default/tslint.json

@@ -0,0 +1,5 @@
+{


please rename the folder to nospace or something like that since there is no default for this rule

ajafff · 2017-06-27T08:38:57Z

src/rules/spaceWithinParensRule.ts

+
+    private checkOpenParenToken(node: ts.Node) {
+        const code = this.sourceFile.text.substr(node.getFullStart());
+        const matchResult = this.openingParenRegex.exec(code);


the regex solution seems a bit hacky and is not very easy to understand.
I'd prefer the following;

// pretending `node` is the OpenParenToken let pos = node.end; let ch = this.sourceFile.text.charCodeAt(pos); while(ts.isWhiteSpaceSingleLine(ch)) { ++pos; ch = this.sourceFile.text.charCodeAt(pos); } const whitespaceCount = pos - node.end; if (this.options.size === 0) { if (whitespaceCount !== 0) // TODO addFailure } else if (!ts.isLineBreak(ch) /* don't require whitespace when followed by line break */ && whitespaceCount !== this.options.size) { // TODO fail }

For the CloseParentToken you need a similar loop from node.pos - 1 backwards.

aminpaks · 2017-06-29T00:36:26Z

I forced push and applied all your comments/notes.
Please review.

ajafff

It's getting better. But there are still some changes needed.
Please bear with me. The number of comments does not mean your code is bad

ajafff · 2017-06-29T11:07:11Z

src/rules/spaceWithinParensRule.ts

+    }
+
+    private checkNode(node: ts.Node): void {
+        forEachToken(node, (token: ts.Node) => {


I actually meant to replace the ts.forEachChild(...) in walk with this call.
You can just take this method's body and put it into walk.

With the current implementation the rule will visit nested tokens multiple times.

ajafff · 2017-06-29T11:07:38Z

src/rules/spaceWithinParensRule.ts

+    };
+    /* tslint:enable:object-literal-sort-keys */
+
+    public static FAILURE_NO_SPACE = "No whitespace within parentheses";


Can you rephrase to Whitespace within parentheses is not allowed. or similar?

ajafff · 2017-06-29T11:08:28Z

src/rules/spaceWithinParensRule.ts

+    /* tslint:enable:object-literal-sort-keys */
+
+    public static FAILURE_NO_SPACE = "No whitespace within parentheses";
+    public static FAILURE_NEEDS_SPACE = "Needs whitespace within parentheses";


Needs x spaces within parentheses.

ajafff · 2017-06-29T11:11:02Z

src/rules/spaceWithinParensRule.ts

+            ++currentPos;
+            currentChar = this.sourceFile.text.charCodeAt(currentPos);
+        }
+        if (!ts.isLineBreak(currentChar)) {


if the rule is configured with 0 spaces, this condition will not show a warning for trailing whitespace right before a line break.
But that will be checked by trailing-whitespace anyway, so it's not strictly necessary here.

I though about it too.

ajafff · 2017-06-29T11:16:32Z

src/rules/spaceWithinParensRule.ts

+    }
+
+    private checkCloseParenToken(tokenNode: ts.Node) {
+        let currentPos = tokenNode.getStart() - 1;


Can be simplified to:

let currentPos = tokenNode.end - 2; // close paren token has a known width of 1, therefore node.getStart() === node.end - 1

Otherwise I would suggest to pass sourceFile as parameter to getStart for performance reasons.

ajafff · 2017-06-29T11:25:26Z

src/rules/spaceWithinParensRule.ts

+    };
+}
+
+class SpaceWhitinParensWalker extends Lint.AbstractWalker<Options> {


s/Whitin/Within/

ajafff · 2017-06-29T11:38:45Z

src/rules/spaceWithinParensRule.ts

+    }
+
+    public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] {
+        return this.applyWithWalker(new SpaceWhitinParensWalker(sourceFile, this.ruleName, parseOptions(Number(this.ruleArguments[0]))));


remove the Number(...), that's done by parseOptions anyway

was getting Unsafe use of expression of type 'any'.

ajafff · 2017-06-29T11:39:34Z

src/rules/spaceWithinParensRule.ts

+    }
+}
+
+function parseOptions(whitespaceSize?: number): Options {


change to whitespaceSize?: any

ajafff · 2017-06-29T11:46:29Z

src/rules/spaceWithinParensRule.ts

+         * it's already been caught by `checkOpenParenToken`
+         */
+        if (!ts.isLineBreak(currentChar) && currentChar !== 40) {
+            const whitespaceCount = tokenNode.end - tokenNode.pos - 1;


const whitespaceCount = token.end - currentPos - 2;

ajafff · 2017-06-29T11:47:12Z

src/rules/spaceWithinParensRule.ts

+    private checkCloseParenToken(tokenNode: ts.Node) {
+        let currentPos = tokenNode.getStart() - 1;
+        let currentChar = this.sourceFile.text.charCodeAt(currentPos);
+        const allowdSpaceCount = this.options.size;


allowedSpaceCount

aminpaks · 2017-06-30T00:31:02Z

I pushed a commit. Please review :)

ajafff

LGTM with minor nits

ajafff · 2017-06-30T10:56:48Z

src/rules/spaceWithinParensRule.ts

+            lintMsg = Rule.FAILURE_NEEDS_SPACE(allowedSpaceCount - whitespaceCount);
+            const whitespace = " ".repeat(allowedSpaceCount - whitespaceCount);
+            lintFix = Lint.Replacement.appendText(position, whitespace);
+        } else if (whitespaceCount > allowedSpaceCount) {


you don't need this if, it's just the else branch

you can also remove the | undefined from the type of the variables above
and you can remove the if (lintMsg !== undefined) check below

ajafff · 2017-06-30T10:57:50Z

src/rules/spaceWithinParensRule.ts

+        }
+    }
+
+    private AddFailureAtWithFix(position: number, length: number, whitespaceCount: number = 0) {


method names should be lowerCamelCase

ajafff · 2017-06-30T10:59:29Z

src/rules/spaceWithinParensRule.ts

+        }
+    }
+
+    private AddFailureAtWithFix(position: number, length: number, whitespaceCount: number = 0) {


the default value for parameter whitespaceCount should not be necessary

aminpaks · 2017-06-30T11:22:05Z

Done

ajafff · 2017-07-21T22:01:30Z

Merging this PR, because it's open for quite some time, code looks good and there was no objection.

Thanks @aminpaks!

Adds new rule "space-within-parens"

75b83e7

ajafff suggested changes Jun 27, 2017

View reviewed changes

aminpaks force-pushed the space-within-parens branch 2 times, most recently from 095d197 to b3aa757 Compare June 29, 2017 00:35

aminpaks force-pushed the space-within-parens branch 2 times, most recently from 1313d78 to 7c62a81 Compare June 29, 2017 00:51

PR comments

72d996a

aminpaks force-pushed the space-within-parens branch from 7c62a81 to 72d996a Compare June 29, 2017 00:53

ajafff suggested changes Jun 29, 2017

View reviewed changes

PR comments 2

74f70bb

aminpaks force-pushed the space-within-parens branch from c7d4352 to 74f70bb Compare June 30, 2017 00:28

ajafff approved these changes Jun 30, 2017

View reviewed changes

PR comments 3

5efb619

ajafff merged commit 524c176 into palantir:master Jul 21, 2017

ajafff mentioned this pull request Jul 29, 2017

Request: Disallow extra spaces directly inside parens #235

Closed

HyphnKnight pushed a commit to HyphnKnight/tslint that referenced this pull request Apr 9, 2018

Adds new rule "space-within-parens" (palantir#2959)

d5fab5e

Adds new rule "space-within-parens" #2959

Adds new rule "space-within-parens" #2959

Conversation

aminpaks commented Jun 26, 2017 • edited Loading

PR checklist

Overview of change:

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

CHANGELOG.md entry:

palantirtech commented Jun 26, 2017

ajafff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aminpaks Jun 27, 2017 • 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

aminpaks Jun 27, 2017 • 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

aminpaks commented Jun 29, 2017

ajafff left a 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

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

aminpaks commented Jun 30, 2017

ajafff left a 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

aminpaks commented Jun 30, 2017

ajafff commented Jul 21, 2017

aminpaks commented Jun 26, 2017 •

edited

Loading

aminpaks Jun 27, 2017 •

edited

Loading

aminpaks Jun 27, 2017 •

edited

Loading