whitespace: add option check-postblock-prekeyword #3308

yubaoquan · 2017-10-11T16:00:30Z

check-postblock-prekeyword

Example:

before lint:

try {
}catch {
}finally {
}

if () {
}else {
}

after lint:

try {
} catch {
} finally {
}


if () {
} else {
}

PR checklist

Addresses an existing issue: Whitespace before curly braces #746
New feature, bugfix, or enhancement
- Includes tests
Documentation update

Overview of change:

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

CHANGELOG.md entry:

[new-rule] check-postblock-prekeyword
[new-rule]

check-postblock-prekeyword

palantirtech · 2017-10-11T16:00:32Z

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

yubaoquan · 2017-10-11T16:13:00Z

signed the individual CLA on October 12, 2017 at 12:03 AM. @palantirtech

ajafff · 2017-10-11T18:40:54Z

src/rules/whitespaceRule.ts

-            * \`"check-preblock"\` checks for whitespace before the opening brace of a block`,
+            * \`"check-preblock"\` checks for whitespace before the opening brace of a block.
+            * \`"check-postblock-prekeyword"\` checks for whitespace between the closing brace of a block
+            and the following keyword (\`} else\` / \`} catch\` / \`} while\`).`,


what's the difference to one-line with "check-whitespace"?

I reviewed the one-line rule code and its test case. Didn't find that rule solve the situation of this PR. And I tried to add one-line with all its options to my test file, the linter didn't report error of missing whitespace between closing bracket and else

#3200 includes the check for whitespace before else.
The only thing missing is whitespace before while in do {...} while (...); loops. But that could easily be added.

So is my pr going to be merged?

I have no preference into which rule the functionality should go. However, we generally try to avoid duplication.
I'll leave that decision to @adidahiya

@yubaoquan do you generally use one-line with "check-whitespace" in your lint config? if so, I think we should just add the additional check @ajafff suggested to one-line instead of a new option here.

ajafff · 2017-10-11T18:43:55Z

src/rules/whitespaceRule.ts

+                        case ts.SyntaxKind.ElseKeyword:
+                        case ts.SyntaxKind.CatchKeyword:
+                        case ts.SyntaxKind.FinallyKeyword:
+                            if (node.end === nextToken.getStart()) {


you can simply use checkForTrailingWhitespace(node.end)

Changed as you said.

ajafff · 2017-10-11T18:59:30Z

src/rules/whitespaceRule.ts

+                        break;
+                    }
+                    switch (nextToken.kind) {
+                        case ts.SyntaxKind.WhileKeyword:


This may do a bit too much. That WhileKeyword could be from the while (...) {} statement following the block.
That may not be a big deal, because it will only complain about such code:

if (condition) { /* code */ }while(condition) {}; ~ [missing whitespace]

Anyway, there's an easier way to do this check without getNextToken:

const parent = node.parent!; if (isDoStatement(parent) || // Block is followed by WhiteKeyword isIfStatement(parent) && parent.elseStatement !== undefined && parent.thenStatement === node || // Block is followed by ElseKeyword isTryStatement(parent) && ( parent.tryBlock === node || // Block is followed by CatchKeyword or FinallyKeyword parent.catchClause !== undefined && parent.catchClause.block === node // Block is followed by FinallyKeyword ) ) { // do stuff }

Well, while writing it down I realized it's not really easier. But it's clearer what kind of statement is checked.

OK. I wrote a function to check conditions as you mentions.

ajafff · 2017-10-12T20:41:47Z

src/rules/whitespaceRule.ts

+            && parent.elseStatement !== undefined
+            && parent.elseStatement !== node;
+        const isTryCatchFinally = utils.isTryStatement(parent)
+            && (parent.tryBlock === node || parent.catchClause === node);


You can use the following to avoid the additional case for CatchClause above (sorry my initial suggestion got that part wrong):

const isTryCatchFinally = utils.isTryStatement(parent) && parent.tryBlock === node || utils.isCatchClause(parent) && parent.parent!.finallyBlock !== undefined;

ajafff · 2017-10-12T20:42:52Z

src/rules/whitespaceRule.ts

@@ -306,7 +320,16 @@ function walk(ctx: Lint.WalkContext<Options>) {
                }
        }
    });
-
+    function matchPostblockPreKeyword(node: ts.Node): boolean {


Consider moving this function out of the walk closure. It doesn't use any variables from that scope, so it doesn't need to be here.

ajafff · 2017-10-12T20:46:58Z

src/rules/whitespaceRule.ts

-            * \`"check-preblock"\` checks for whitespace before the opening brace of a block`,
+            * \`"check-preblock"\` checks for whitespace before the opening brace of a block.
+            * \`"check-postblock-prekeyword"\` checks for whitespace between the closing brace of a block
+            and the following keyword (\`} else\` / \`} catch\` / \`} while\`).`,


I have no preference into which rule the functionality should go. However, we generally try to avoid duplication.
I'll leave that decision to @adidahiya

1. move function `matchPositionblockPreKeyword` out of walk closure 2. update logic of checking for finally clause in `matchPositionblockPreKeyword` 3. remove check for catch block in walk

jkillian · 2018-05-01T22:06:29Z

it's a little confusing, but functionality should go in the one-line rule likely, see #3308 (comment)

Add Feature

5f9ae8d

check-postblock-prekeyword

ajafff reviewed Oct 11, 2017

View reviewed changes

Optimize code follow what reviewer said

9c3b6bb

ajafff changed the title ~~Add Feature~~ whitespace: add option check-postblock-prekeyword Oct 12, 2017

ajafff reviewed Oct 12, 2017

View reviewed changes

Update

35a43c5

1. move function `matchPositionblockPreKeyword` out of walk closure 2. update logic of checking for finally clause in `matchPositionblockPreKeyword` 3. remove check for catch block in walk

jkillian closed this May 1, 2018

jkillian added the Resolution: Aged Away label May 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

whitespace: add option check-postblock-prekeyword #3308

whitespace: add option check-postblock-prekeyword #3308

yubaoquan commented Oct 11, 2017 •

edited

Loading

palantirtech commented Oct 11, 2017

yubaoquan commented Oct 11, 2017

ajafff Oct 11, 2017

yubaoquan Oct 12, 2017

ajafff Oct 12, 2017

yubaoquan Oct 12, 2017

ajafff Oct 12, 2017

adidahiya Nov 28, 2017

ajafff Oct 11, 2017

yubaoquan Oct 12, 2017

ajafff Oct 11, 2017

yubaoquan Oct 12, 2017

ajafff Oct 12, 2017

yubaoquan Oct 13, 2017

ajafff Oct 12, 2017

yubaoquan Oct 13, 2017

ajafff Oct 12, 2017

jkillian commented May 1, 2018

whitespace: add option check-postblock-prekeyword #3308

whitespace: add option check-postblock-prekeyword #3308

Conversation

yubaoquan commented Oct 11, 2017 • edited Loading

Example:

PR checklist

Overview of change:

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

CHANGELOG.md entry:

palantirtech commented Oct 11, 2017

yubaoquan commented Oct 11, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkillian commented May 1, 2018

yubaoquan commented Oct 11, 2017 •

edited

Loading