-
Notifications
You must be signed in to change notification settings - Fork 889
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a quick first review. The code is a bit difficult to follow. I will do a more thorough review in the next days (hopefully tomorrow).
src/rules/indentRule.ts
Outdated
} | ||
|
||
function recursiveGetNodeAt(root: ts.Node, pos: number): ts.Node | undefined { | ||
let result: ts.Node | undefined = root.getChildAt(pos); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this does not what you intended. it gets the n
th child, not the child at position n
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It just works. Not found a test case can prove this usage is wrong.
src/rules/indentRule.ts
Outdated
if (node.getFullStart() < pos && node.getEnd() >= pos) { | ||
result = recursiveGetNodeAt(node, pos); | ||
} | ||
ts.forEachChild(node, cb); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, code removed
src/rules/indentRule.ts
Outdated
const currentNodePos: number = pos + indentEnd; | ||
let currentNode: ts.Node | undefined = recursiveGetNodeAt(sourceFile, currentNodePos); | ||
let depth = 0; | ||
if (currentNode !== undefined && inTsx(sourceFile)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to do nothing at all in .tsx files? And should the same apply to .jsx?
If there is a good reason, you could consider moving this check to the top of walk
to avoid unnecessary work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the check for jsx syntax.
src/rules/indentRule.ts
Outdated
indentEnd: number; | ||
line: string; | ||
sourceFile: ts.SourceFile; | ||
}): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider moving this function inside walk
so it closes over the variables like ctx
, tabs
, sourceFile
and so on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/rules/indentRule.ts
Outdated
ts.SyntaxKind.TemplateHead, | ||
ts.SyntaxKind.TemplateMiddle, | ||
ts.SyntaxKind.TemplateTail, | ||
].indexOf(node.kind) > -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
node.kind >= ts.SyntaxKind.FirstTemplateToken && node.kind <= ts.SyntaxKind.LastTemplateToken
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you wish
src/rules/indentRule.ts
Outdated
let result = 0; | ||
let parent: ts.Node | undefined = node.parent; | ||
const blockTypes: ts.SyntaxKind[] = [ | ||
ts.SyntaxKind.Block, // 207 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these numbers are subject to change with future versions of typescript. you can remove them, because they do not add any value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
src/rules/indentRule.ts
Outdated
ts.SyntaxKind.CaseBlock, // 235 | ||
ts.SyntaxKind.EnumMember, // 264 | ||
ts.SyntaxKind.CaseClause, // 257 | ||
// ts.SyntaxKind.IfStatement, // Have no idea on how to check whether a node is in if paren |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you probably also need ForStatement
, ForOfStatement
, ForInStatement
, WhileStatement
, DoStatement
and WithStatement
but only if they do not contain a block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added another function to check text in parens of these statements
docs/_data/formatters.json
Outdated
@@ -0,0 +1,83 @@ | |||
[ |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
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
Note that there is already another open PR that tackles the same issue: #3300 |
How's the progress? |
Any problems with this pr? @ajafff |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides my comments, please add some more comments to describe what the code does and why it needs to do it. As it is now, the code is pretty hard to follow.
docs/_data/formatters.json
Outdated
@@ -0,0 +1,83 @@ | |||
[ |
There was a problem hiding this comment.
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
src/rules/indentRule.ts
Outdated
const currentNodePos: number = pos + indentEnd; | ||
let currentNode: ts.Node | undefined = recursiveGetNodeAt(sourceFile, currentNodePos); | ||
let depth = 0; | ||
if (currentNode !== undefined && currentNode.getStart() === currentNodePos) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/rules/indentRule.ts
Outdated
const passIndentDepthCheck = inStringTemplate(currentNode) || (line.trim().charAt(0) === "*"); | ||
|
||
if (!passIndentDepthCheck && expectedIndentation !== actualIndentation) { | ||
failure = Rule.FAILURE_STRING(tabs ? "tab" : size === undefined ? "space" : `${depth * size} space`); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
src/rules/indentRule.ts
Outdated
const result: ts.Node | undefined = root.getChildAt(pos); | ||
if (result === undefined) { | ||
ts.forEachChild(root, function cb(node: ts.Node): void { | ||
ts.forEachChild(node, cb); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
src/rules/indentRule.ts
Outdated
@@ -121,6 +159,130 @@ function walk(ctx: Lint.WalkContext<Options>): void { | |||
} | |||
} | |||
|
|||
function recursiveGetNodeAt(root: ts.Node, pos: number): ts.Node | undefined { | |||
const result: ts.Node | undefined = root.getChildAt(pos); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
src/rules/indentRule.ts
Outdated
const expectedIndentation: string = tabs ? "\t".repeat(depth) | ||
: " ".repeat(size * depth); | ||
const actualIndentation = line.slice(0, indentEnd); | ||
const passIndentDepthCheck = inStringTemplate(currentNode) || (line.trim().charAt(0) === "*"); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/rules/indentRule.ts
Outdated
@@ -121,6 +159,130 @@ function walk(ctx: Lint.WalkContext<Options>): void { | |||
} | |||
} | |||
|
|||
function recursiveGetNodeAt(root: ts.Node, pos: number): ts.Node | undefined { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
src/rules/indentRule.ts
Outdated
result++; | ||
} | ||
if (inStatementParen(parent)) { | ||
result ++; |
There was a problem hiding this comment.
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();
There was a problem hiding this comment.
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.
src/rules/indentRule.ts
Outdated
if ( | ||
node.parent !== undefined | ||
&& node.parent.kind === ts.SyntaxKind.CallExpression | ||
&& /^\s*\./.test(line) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/rules/indentRule.ts
Outdated
); | ||
} | ||
|
||
function nodeAtOutside(node: ts.Node): boolean { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@yubaoquan sorry for the delay, I got distracted. Unfortunately writing new code is a lot more fun than reviewing someone else's code. |
Updated the code as you said. Thank you for your review and advices! |
Something is wrong with CI, how to fix it? Should I merge the master branch into this branch?
|
Exactly. This is fixed on master, you just need to merge |
how's it going? |
Do we have an estimate of when this might be merged? @ajafff @adidahiya |
@yubaoquan before jumping right back to reviewing the code, I tried how it works. There are several false positives that need to be addressed: multiline arguments: foo(
bar, // 0 space indentation expected
baz, // 0 space indentation expected
);
new Foo(
bar, // 0 space indentation expected
baz, // 0 space indentation expected
); continuation on the next line: let someReallyLongVariableName =
anotherReallyLongVariableName; // 0 space indentation expected
return foo
&& bar // 0 space indentation expected
&& baz; // 0 space indentation expected
someArray.filter((element) =>
doStuff(element)); // 0 space indentation expected
// or
const someFn = (element) =>
doStuff(element); // 0 space indentation expected multiline named imports: import {
foo, // 0 space indentation expected
bar, // 0 space indentation expected
} from "foobar";
// probably the same for 'export'
export {
foo, // 0 space indentation expected
bar, // 0 space indentation expected
}; else-if: if (foo) {
foo;
} else if (bar) {
bar; // 8 space indentation expected
} // 4 space indentation expected type literals: export interface TestResult {
directory: string;
results: {
[fileName: string]: TestOutput | SkippedTest; // 4 space indentation expected
};
} ParenthesizedExpression: return (
foo // 0 space indentation expected
); template expressions: `foo ${
foo // 0 space indentation expected
}` template literals: foo(
`foo`, // this line is valid
// next line should have an error, currently it is ignored
`
bar
`,
// the two lines above are valid, because the indent is inside the template string
); indent wrongly calculated at start of statement: // next line starts with one space
for(;;) {} // 4 space indentation expected Of course everything mentioned above is open for discussion. |
All cases fixed except two below:
What's more, if you got a |
IMO the opening backtick of a template string should be checked like every other token. We just ignore the indent inside the template string, because that could be part of the content.
The |
Here we go with the next round of false positives: const linter = new Linter({
fix: false,
formatter: "prose",
}); // 12 spaces indentation expected - currently 8 still a problem with continuation on the next line: const expectedResult =
getFailureString(TEST_FILE, 1, 1, "first failure", "first-name") + // 8 space indentation expected - currently 12
getFailureString(TEST_FILE, 2, 12, "mid failure", "mid-name") + // 8 space indentation expected - currently 12
getFailureString(TEST_FILE, 9, 2, "last failure", "last-name"); // 8 space indentation expected - currently 12 chained method calls: fs.createReadStream("test/files/multiple-fixes-test/multiple-fixes.test.ts")
.pipe(fs.createWriteStream(tempFile))
.on("finish", () => {
doStuff(); // 4 spaces indentation expected
}); // 0 spaces indentation expected blocks in CaseClause and DefaultClause case ts.SyntaxKind.BinaryExpression: {
break; // 20 spaces indentation expected - currently 16
} // 20 spaces indentation expected - currently 12 multiline arrow function as argument: return find(sym.declarations, (decl) =>
isClassLikeDeclaration(decl) ? decl.typeParameters : undefined); // 12 - currenly 8 property initializer on the next line: class Foo {
public static FAILURE_STRING_TRAILING =
"Did not expect single-line type literal to have a trailing ';'."; // 4 spaces indentation expected -
currently 8
} something with TypeAlias: export type Location =
| ts.PrefixUnaryExpression // 0 spaces indentation expected (for all of them)
| ts.IfStatement
| ts.WhileStatement
| ts.DoStatement
| ts.ForStatement
| ts.ConditionalExpression
| ts.BinaryExpression; something with abstract class: // only reproducible if the class declaration is not on the first line
abstract class Foo {} // 4 spaces indentation expected multiline type arguments: type T = Map<
string, // 0 space indentation expected
string // 0 space indentation expected
>; I like how you handle nested conditional expressions: return foo
? foo
: bar
? bar
: baz; Though others prefer the following, which is now invalid. I don't have a strong opinion for one of them. return foo ? foo
: bar ? bar
: baz; I actually feel kind of sorry for you, because your first contribution turns out to be that difficult. I really appreciate the effort you put into this PR. |
My logic of calculating the depth of code after chaining call is not good. I need time to refactor that part. Recently pretty busy, so it will take weeks before committing next pr. |
refactor chaining call check and add other test cases
Pretty hard to cover all the code styles. Pretty hard to debug this time. Anyway, fixed all cases you mentioned last time, and added the corresponding test cases. As for the conditional expressions, I have no idea, should it to be an configurable option in this rule? |
…ure-indent-size-check Fix circleCI error
test/rules/trailing-comma/singleline-always/test.ts.lint: Failed! |
You cannot fix the failing test, see #3494 (comment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty hard to cover all the code styles. Pretty hard to debug this time.
Yep, which is why I personally prefer to leave such formatting to tools like prettier
:)
src/rules/indentRule.ts
Outdated
return result; | ||
} | ||
|
||
// function isSameLineWithConditionalExpr(sourceFile: ts.SourceFile, nodePos: number, parent: ts.Node): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove commented code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/rules/indentRule.ts
Outdated
foo() | ||
.then() | ||
In this case, `.then` should have one more indent. | ||
Maybe this should depend on a configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use proper JSDoc formatting please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, is this last line a question for code reviewers? I don't think this is a helpful comment without more elaboration. Either remove it or make the "maybe" part more clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/rules/indentRule.ts
Outdated
* // xxx | ||
* } | ||
* In this case, the `Foo` is regarded as should have the same indent depth with `class`. | ||
* Should it have one more depth? I'm not sure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use proper JSDoc formatting -- there is an extra whitespace after each *
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
…ure-indent-size-check merge master
Is that a -1 on this PR? If so, we can stop here and instead point people to |
Sorry, no, it's not an explicit -1 on this PR. There's a lot of demand for this feature in core TSLint, so it makes sense to add it, but we (project collaborators) might provide a lower amount of support for it than other core features. It might even deserve a callout in the docs that says the fixer for this rule is "experimental" (defer to your judgement here -- I haven't looked very closely at the CR progress and the false positives identified here). I think in the future we should break up the core rule set into multiple packages, one of which would be formatting rules. Users of TSLint + Prettier would be able to disable the formatting rules all at once and we could set users' expectations of support accordingly. But of course that's out of scope for this PR. |
When can this be merged? Or lets think more test cases? @ajafff |
Ok, let's continue: const expectedResult =
`foo`; // 0 spaces expected
return execRunner(
{
foo,
}, // 0 spaces expected
);
const options = {
foo; // 8 spaces expected
}; // 4 spaces expected
type Options = Record< // 4 spaces expected
"branch" | "decl" | "operator" | "module" | "separator" | "restSpread" | "type" | "typecast" | "typeOperator" | "preblock",
boolean>;
type Foo = boolean; // 4 spaces expected
return (a.dotDotDotToken !== undefined) === (b.dotDotDotToken !== undefined) &&
(a.questionToken !== undefined) === (b.questionToken !== undefined); // 0 spaces expected
const couldBeSwitch = everyCase(node, (expr) => {
casesSeen++;
if (switchVariable !== undefined) {
return nodeEquals(expr, switchVariable, sourceFile);
} else {
switchVariable = expr; // 12 spaces expected
return true;
}
});
doStuff([
foo, // 8 spaces expected
]); // 4 spaces expected I think that's it for now. I think we better add a new option for checking the indent level. This new behavior could make a lot users insane when enabled by default. |
Done. New test cases added, bug fixed. |
I think a better way to check indent is making a tree, each line of the source file corresponds to a leaf on the tree. The depth of the leaf is the indent depth of the line. So the whole thing would be much easier. But I'm not good at making such a tree. |
is it almost ready to merge now? any estimation on when this can get merged? |
@ajafff It's time to find new bugs, need your report! thanks :) |
@yubaoquan @ajafff I'd really love to see this feature released, so for what it's worth:
Of course there are many variations of nested blocks, and statement continuations, and so on -- thus I can't try everything, but as far as I did try this seems to work well! |
Tslint indent
@yubaoquan please update this branch so we can review it, or close it if no longer relevant. we will close this if we do not hear from you in two weeks. I am inclined to reject this feature for two reasons:
|
Closing due to age and inactivity. |
PR checklist
Overview of change:
Add ability to check the indent depth.
For example:
Is there anything you'd like reviewers to focus on?
Maybe not all use cases in the test files are covered.
CHANGELOG.md entry:
[new-fixer]
[enhancement]