Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Feature indent depth check #3395

Closed
wants to merge 19 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 83 additions & 0 deletions docs/_data/formatters.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
[
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you merge latest master these generated files should be gone

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for some strange resons the files still show up in the diff. please git rm docs/formatters/stylish/index.html docs/_data/formatters.json

{
"formatterName": "checkstyle",
"description": "Formats errors as through they were Checkstyle output.",
"descriptionDetails": "\nImitates the XMLLogger from Checkstyle 4.3. All failures have the 'warning' severity.",
"sample": "\n<?xml version=\"1.0\" encoding=\"utf-8\"?>\n<checkstyle version=\"4.3\">\n <file name=\"myFile.ts\">\n <error line=\"1\" column=\"14\" severity=\"warning\" message=\"Missing semicolon\" source=\"failure.tslint.semicolon\" />\n </file>\n</checkstyle>",
"consumer": "machine"
},
{
"formatterName": "codeFrame",
"description": "Framed formatter which creates a frame of error code.",
"descriptionDetails": "\nPrints syntax highlighted code in a frame with a pointer to where\nexactly lint error is happening.",
"sample": "\nsrc/components/Payment.tsx\nParentheses are required around the parameters of an arrow function definition (arrow-parens)\n 21 | public componentDidMount() {\n 22 | this.input.focus();\n> 23 | loadStripe().then(Stripe => Stripe.pay());\n | ^\n 24 | }\n 25 |\n 26 | public render() {",
"consumer": "human"
},
{
"formatterName": "filesList",
"description": "Lists files containing lint errors.",
"sample": "directory/myFile.ts",
"consumer": "machine"
},
{
"formatterName": "json",
"description": "Formats errors as simple JSON.",
"sample": "\n[\n {\n \"endPosition\": {\n \"character\": 13,\n \"line\": 0,\n \"position\": 13\n },\n \"failure\": \"Missing semicolon\",\n \"fix\": {\n \"innerStart\": 13,\n \"innerLength\": 0,\n \"innerText\": \";\"\n },\n \"name\": \"myFile.ts\",\n \"ruleName\": \"semicolon\",\n \"startPosition\": {\n \"character\": 13,\n \"line\": 0,\n \"position\": 13\n }\n }\n]",
"consumer": "machine"
},
{
"formatterName": "junit",
"description": "Formats errors as through they were JUnit output.",
"descriptionDetails": "\nImitates the JUnit XML Output",
"sample": "\n<?xml version=\"1.0\" encoding=\"utf-8\"?>\n<testsuites package=\"tslint\">\n <testsuite name=\"myFile.ts\">\n <testcase name=\"Line 1, Column 14: semicolon\">\n <failure type=\"warning\">Missing semicolon</failure>\n </testcase>\n </testsuite>\n</testsuites>\n",
"consumer": "machine"
},
{
"formatterName": "msbuild",
"description": "Formats errors for consumption by msbuild.",
"descriptionDetails": "The output is compatible with both msbuild and Visual Studio.",
"sample": "myFile.ts(1,14): warning: Missing semicolon",
"consumer": "machine"
},
{
"formatterName": "pmd",
"description": "Formats errors as through they were PMD output.",
"descriptionDetails": "Imitates the XML output from PMD. All errors have a priority of 1.",
"sample": "\n<pmd version=\"tslint\">\n <file name=\"myFile.ts\">\n <violation begincolumn=\"14\" beginline=\"1\" priority=\"3\" rule=\"Missing semicolon\"></violation>\n </file>\n</pmd>",
"consumer": "machine"
},
{
"formatterName": "prose",
"description": "The default formatter which outputs simple human-readable messages.",
"sample": "ERROR: myFile.ts[1, 14]: Missing semicolon",
"consumer": "human"
},
{
"formatterName": "stylish",
"description": "Human-readable formatter which creates stylish messages.",
"descriptionDetails": "\nThe output matches what is produced by ESLint's stylish formatter.\nIts readability is enhanced through spacing and colouring.",
"sample": "\nmyFile.ts\n1:14 semicolon Missing semicolon",
"consumer": "human"
},
{
"formatterName": "tap",
"description": "Formats output as TAP stream.",
"descriptionDetails": "Provides error messages output in TAP13 format which can be consumed by any TAP formatter.",
"sample": "\nTAP version 13\n1..1\nnot ok 1 - Some error\n ---\n message: Variable has any type\n severity: error\n data:\n ruleName: no-any\n fileName: test-file.ts\n line: 10\n character: 10\n failureString: Some error\n rawLines: Some raw output\n ...",
"consumer": "machine"
},
{
"formatterName": "verbose",
"description": "The human-readable formatter which includes the rule name in messages.",
"descriptionDetails": "The output is the same as the prose formatter with the rule name included",
"sample": "ERROR: (semicolon) myFile.ts[1, 14]: Missing semicolon",
"consumer": "human"
},
{
"formatterName": "vso",
"description": "Formats output as VSO/TFS logging commands.",
"descriptionDetails": "\nIntegrates with Visual Studio Online and Team Foundation Server by outputting errors\nas 'warning' logging commands.",
"sample": "##vso[task.logissue type=warning;sourcepath=myFile.ts;linenumber=1;columnnumber=14;code=semicolon;]Missing semicolon",
"consumer": "machine"
}
]
15 changes: 15 additions & 0 deletions docs/formatters/stylish/index.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
---
formatterName: stylish
description: Human-readable formatter which creates stylish messages.
descriptionDetails: |-

The output matches what is produced by ESLint's stylish formatter.
Its readability is enhanced through spacing and colouring.
sample: |-

myFile.ts
1:14 semicolon Missing semicolon
consumer: human
layout: formatter
title: 'TSLint formatter: stylish'
---
177 changes: 176 additions & 1 deletion src/rules/indentRule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,12 +100,50 @@ interface Options {
function walk(ctx: Lint.WalkContext<Options>): void {
const { sourceFile, options: { tabs, size } } = ctx;
const regExp = tabs ? new RegExp(" ".repeat(size === undefined ? 1 : size)) : /\t/;
const failure = Rule.FAILURE_STRING(tabs ? "tab" : size === undefined ? "space" : `${size} space`);
let failure = Rule.FAILURE_STRING(tabs ? "tab" : size === undefined ? "space" : `${size} space`);

for (const {pos, contentLength} of getLineRanges(sourceFile)) {
if (contentLength === 0) { continue; }
const line = sourceFile.text.substr(pos, contentLength);
let indentEnd = line.search(/\S/);
// <check-indent-depth>
if (size !== undefined) {
const currentNodePos: number = pos + indentEnd;
let currentNode: ts.Node | undefined = recursiveGetNodeAt(sourceFile, currentNodePos);
let depth = 0;
if (currentNode !== undefined && currentNode.getStart() === currentNodePos) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider passing sourceFile as parameter to .getStart() for performance reasons

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

depth = getNodeDeepth(currentNode, line);
} else {
currentNode = recursiveGetNodeAtEnd(sourceFile, currentNodePos);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this function ever return something other than undefined?
You search for a node that ends at the end of the indentation. A node ends before any whitespace or comments after it. That means even if the indentation is 0, the last node ends on the previous line, therefore currentNodePos - node.end will always be 1 or 2 (depending on the line break style)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In if block, the condition matches the opening part of a block, like module TestModule {;
In else block, the condition matches the closing part of a block, like a single }.

if (currentNode !== undefined) {
depth = getNodeDeepth(currentNode, line);
}
}
if (currentNode !== undefined) {
const expectedIndentation: string = tabs ? "\t".repeat(depth)
: " ".repeat(size * depth);
const actualIndentation = line.slice(0, indentEnd);
const passIndentDepthCheck = inStringTemplate(currentNode) || (line.trim().charAt(0) === "*");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(line.trim().charAt(0) === "*") is used to check for multiline JSDoc comments?
If yes, this needs to be updated to also support multiline comments without a leading asterisk. You can simply use isPositionInComment from the tsutils package

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


if (!passIndentDepthCheck && expectedIndentation !== actualIndentation) {
failure = Rule.FAILURE_STRING(tabs ? "tab" : size === undefined ? "space" : `${depth * size} space`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

size can never be undefined in this block

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

ctx.addFailureAt(
pos,
indentEnd,
failure,
createIndentSizeFix(
pos,
tabs,
size,
depth,
indentEnd,
),
);
continue;
}
}
}
// </check-indent-depth>
if (indentEnd === 0) { continue; }
if (indentEnd === -1) {
indentEnd = contentLength;
Expand All @@ -121,6 +159,130 @@ function walk(ctx: Lint.WalkContext<Options>): void {
}
}

function recursiveGetNodeAt(root: ts.Node, pos: number): ts.Node | undefined {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for your use case you can probably just use getTokenAtPosition from the tsutils package

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried, the replacement produce wrong results. The getTokenAtPosition will always return a token, which is not expected. In recursiveGetNodeAt, the getChildAt will return undefined on some positions, and then the function should return the root node. If replace to getTokenAtPosition, the returned value will be unexpected. Anyway I have refactored this function.

const result: ts.Node | undefined = root.getChildAt(pos);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't think this is right. Since root is always the SourceFile, getChildAt will return a SyntaxList with all statements when pos === 0 and EndOfFileToken when pos === 1. In all other cases it just returns undefined

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated. The recursiveGetNodeAt and recursiveGetNodeAt are not suitable names with the logic, and the inner logic is not clearly, so I refactor theses two function to one single function recursiveGetNodeAt.
The function just find the node of current line. If the pos is between the getFullStart and getEnd, that means the line is the node or is child of the node.

if (result === undefined) {
ts.forEachChild(root, function cb(node: ts.Node): void {
ts.forEachChild(node, cb);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you won't find the node if you recurse here, because the parent is already out of the range

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, code removed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is actually a no-op, because the callback never modifies result

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

});
}
if (result !== undefined) {
return result;
}
return root;
}

function recursiveGetNodeAtEnd(root: ts.Node, pos: number): ts.Node | undefined {
if (root.getEnd() === pos) {
return root;
}
let result: ts.Node | undefined;
ts.forEachChild(root, function cb(node: ts.Node): void {
if (node.getFullStart() < pos && node.getEnd() >= pos) {
result = recursiveGetNodeAt(node, pos);
}
ts.forEachChild(node, cb);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this line can be removed

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is used to find the closing bracket line. Should not be removed.

});
return result;
}

function getNodeDeepth(node: ts.Node, line: string): number {
let result = 0;
let parent: ts.Node | undefined = node.parent;
const blockTypes: ts.SyntaxKind[] = [
ts.SyntaxKind.Block,
ts.SyntaxKind.Parameter,
ts.SyntaxKind.CaseBlock,
ts.SyntaxKind.EnumMember,
ts.SyntaxKind.CaseClause,
ts.SyntaxKind.JsxElement,
ts.SyntaxKind.ModuleBlock,
ts.SyntaxKind.TemplateSpan,
ts.SyntaxKind.DefaultClause,
ts.SyntaxKind.ClassDeclaration,
ts.SyntaxKind.ArrayLiteralExpression,
ts.SyntaxKind.ObjectLiteralExpression,
];
if (isCloseElement(node, line)) {
result --;
}
if (isCloseParen(node, line)) {
result ++;
}
if (inStatementParen(node)) {
result ++;
}
while (parent !== undefined) {
if (blockTypes.indexOf(parent.kind) > -1) {
result++;
}
if (inStatementParen(parent)) {
result ++;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some special cases to consider:

// multiline condition, works well for `if`, but probably doesn't for `while`
while(foo() &&
      bar() &&
      baz()) {
    doStuff;
}
// might not be worth fixing, because it can be reformatted to
while(
    foo() &&
    bar() &&
    baz()
) {
    doStuff;
}

// statement continuation on the next line
let foo = bar
    ? baz
    : bas;
foo = "some"
    + "really"
    + "long"
    + "string";

// multiline property access
foo
    .bar
    .baz();

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, add these into test cases. Test passed.

}
parent = parent.parent;
}
if (nodeAtOutside(node)) {
result--;
}
if (
node.parent !== undefined
&& node.parent.kind === ts.SyntaxKind.CallExpression
&& /^\s*\./.test(line)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add a comment explaining what this condition does. it took me a while to figure it out

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

) {
result ++;
}
return result;
}

function inStatementParen(node: ts.Node): boolean {
if (node.parent === undefined) {
return false;
}
const parentInstatment = [
ts.SyntaxKind.IfStatement,
ts.SyntaxKind.WhileStatement,
ts.SyntaxKind.ForStatement,
ts.SyntaxKind.ForOfStatement,
ts.SyntaxKind.DoStatement,
ts.SyntaxKind.WithStatement,
].indexOf(node.parent.kind) >= 0;
return node.kind !== ts.SyntaxKind.Block && parentInstatment;
}

function isCloseParen(node: ts.Node, line: string): boolean {
if (node.kind !== ts.SyntaxKind.Block) {
return false;
}
return /^\s*\)/.test(line);
}

function isCloseElement(node: ts.Node, line: string): boolean {
if (node.parent === undefined) {
return false;
}
return node.parent.kind === ts.SyntaxKind.JsxElement
&& /^\s*<\//.test(line);
}

function inStringTemplate(node: ts.Node | undefined): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

node is never undefined when you call this function

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

if (node === undefined) {
return false;
}
return node.kind === ts.SyntaxKind.NoSubstitutionTemplateLiteral
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NoSubstitutionTemplateLiteral is actually the same as FirstTemplateToken

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

|| (
node.kind >= ts.SyntaxKind.FirstTemplateToken
&& node.kind <= ts.SyntaxKind.LastTemplateToken
);
}

function nodeAtOutside(node: ts.Node): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add a comment to describe what this function does

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

if (node.parent === undefined) {
return false;
}
return node.kind === ts.SyntaxKind.Identifier
&& node.parent.kind === ts.SyntaxKind.ClassDeclaration;
}

function createFix(lineStart: number, fullLeadingWhitespace: string, tabs: boolean, size?: number): Lint.Fix | undefined {
if (size === undefined) { return undefined; }
const replaceRegExp = tabs
Expand All @@ -131,3 +293,16 @@ function createFix(lineStart: number, fullLeadingWhitespace: string, tabs: boole
(tabs ? "\t" : " ".repeat(size)).repeat(Math.ceil(match.length / size)));
return new Lint.Replacement(lineStart, fullLeadingWhitespace.length, replacement);
}

function createIndentSizeFix(
lineStart: number,
useTab: boolean,
tabSize: number,
indentTimes: number,
len: number,
): Lint.Fix {
const replacement = useTab ?
"\t".repeat(indentTimes) :
" ".repeat(tabSize * indentTimes);
return new Lint.Replacement(lineStart, len, replacement);
}
Loading