Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

List position based formatting #28340

Merged
merged 11 commits into from
Nov 9, 2018

Conversation

saschanaz
Copy link
Contributor

Revives #13574

Fixes #5830, fixes #5890, fixes #6252, fixes #6320, fixes #6451, fixes #10681, fixes #26906

tests/cases/fourslash/formattingOnChainedCallbacks.ts Outdated Show resolved Hide resolved
@@ -323,112 +322,109 @@ namespace ts.formatting {
return false;
}

function getListIfVisualStartEndIsInListRange(list: NodeArray<Node> | undefined, start: number, end: number, node: Node) {
Copy link

Choose a reason for hiding this comment

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

function getListIfVisualStartEndIsInListRange(list: NodeArray<Node> | undefined, start: number, end: number, node: Node, sourceFile: SourceFile): NodeArray<Node> | undefined {
    return list && rangeContainsStartEnd(getListRange(node, list, sourceFile), start, end) ? list : undefined;
}
function getListRange(node: Node, list: TextRange, sourceFile: SourceFile): TextRange {
    const children = node.getChildren(sourceFile);
    for (let i = 1; i < children.length - 1; i++) {
        if (children[i].pos === list.pos && children[i].end === list.end) {
            return { pos: children[i - 1].end, end: children[i + 1].getStart(sourceFile) };
        }
    }
    return list;
}
  • Should pass in sourceFile to some methods for performance.
  • width = end - start, so end - width = start
  • What does "visual" mean here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NodeArray starts at the previous token's end positions and ends at the last list item's end position. So think about the following code:

func(
  // visually the list starts at line 0 and ends in line 2, but actually it ends at line 0
);

func(
  1,
  // visually the list starts at line 0 and ends in line 3, but actually it ends at line 1
  // which means the position of these comments does not belong to the list range.
);

src/services/formatting/formatting.ts Outdated Show resolved Hide resolved
src/services/formatting/formatting.ts Outdated Show resolved Hide resolved
switch (node.kind) {
case SyntaxKind.TypeReference:
return getListIfVisualStartEndIsInListRange((<TypeReferenceNode>node).typeArguments, start, end, node);
return getListIfVisualStartEndIsInListRange((<TypeReferenceNode>node).typeArguments, start, end, node, sourceFile);
Copy link

Choose a reason for hiding this comment

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

You could create a closure so you don't have to repeat the arguments start, end, node, sourceFile everywhere.

@DanielRosenwasser
Copy link
Member

I'm pulling this in, and if we'd like to address any changes related to code cleanliness or perf we can do this after. Thank you so much for sticking it out and sending out these changes again.

@DanielRosenwasser
Copy link
Member

⭐️ ⭐️ ⭐️ ⭐️ ⭐️ ❤️

@DanielRosenwasser DanielRosenwasser merged commit 02ca5be into microsoft:master Nov 9, 2018
@saschanaz saschanaz deleted the listindent-revive branch November 10, 2018 02:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment