GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — List position based formatting by saschanaz · Pull Request #28340 · microsoft/TypeScript

Conversation

@saschanaz

@saschanaz Kagami Sascha Rosylight (saschanaz) commented Nov 5, 2018

Copy link
Copy Markdown
Contributor

Revives #13574

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

zpdDG4gta8XKpMCd, localvoid, SlurpTheo, iuliust, and yume-chan reacted with thumbs up emoji zpdDG4gta8XKpMCd reacted with hooray emoji zpdDG4gta8XKpMCd and iuliust reacted with heart emoji
All reactions
  • 👍 5 reactions
  • 🎉 1 reaction
  • ❤️ 2 reactions
Kagami Sascha Rosylight (saschanaz) added 10 commits November 5, 2018 21:45
@saschanaz
use last line indentation on formatter
7561ec0
... and add explicit delta suppressor for list end tokens
@saschanaz
remove deprecated function
d7f1935
@saschanaz
add getListByPosition
cd19a89
@saschanaz
indent using list start position before first list item
655bf20
@saschanaz
indent using list start position
ac51733
@saschanaz
tests
90d5413
@saschanaz
resolve TODO
c6174f6
@saschanaz
indent size 8 based on list startline indentation
7bc40ce
@saschanaz
strict types
7fa447b
@saschanaz
adjust test: always indented inside a list
8ed21a8
@saschanaz Kagami Sascha Rosylight (saschanaz) mentioned this pull request Nov 5, 2018
Revive PR #13574 #26906
Closed
@weswigham Wesley Wigham (weswigham) assigned Sheetal Nandi (sheetalkamat) Nov 6, 2018
@weswigham
Wesley Wigham (weswigham) requested a review from Sheetal Nandi (sheetalkamat) November 6, 2018 21:44
ghost
Deleted user (ghost) reviewed Nov 8, 2018
View reviewed changes
Comment thread tests/cases/fourslash/formattingOnChainedCallbacks.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread src/services/formatting/smartIndenter.ts Outdated
return false;
}

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

@ghost Deleted user (ghost) Nov 8, 2018

Copy link
Copy Markdown

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.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@saschanaz Kagami Sascha Rosylight (saschanaz) Nov 9, 2018

Copy link
Copy Markdown
Contributor Author

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.

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.
);

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread src/services/formatting/formatting.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread src/services/formatting/formatting.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

@saschanaz
apply suggested changes
7995f91
ghost
Deleted user (ghost) reviewed Nov 9, 2018
View reviewed changes
Comment thread src/services/formatting/smartIndenter.ts
switch (node.kind) {
case SyntaxKind.TypeReference:
return getListIfVisualStartEndIsInListRange((<TypeReferenceNode>node).typeArguments, start, end, node);
return getListIfVisualStartEndIsInListRange((<TypeReferenceNode>node).typeArguments, start, end, node, sourceFile);

@ghost Deleted user (ghost) Nov 9, 2018

Copy link
Copy Markdown

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 could create a closure so you don't have to repeat the arguments start, end, node, sourceFile everywhere.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
sheetalkamat
Sheetal Nandi (sheetalkamat) approved these changes Nov 9, 2018
View reviewed changes
@DanielRosenwasser

Daniel Rosenwasser (DanielRosenwasser) commented Nov 9, 2018

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@DanielRosenwasser

Daniel Rosenwasser (DanielRosenwasser) commented Nov 9, 2018

Copy link
Copy Markdown
Member

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

saschanaz reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser) merged commit 02ca5be into microsoft:master Nov 9, 2018
@ghost ghost mentioned this pull request Nov 10, 2018
Reduce duplicate code in getListByRange #28455
Merged
@saschanaz
Kagami Sascha Rosylight (saschanaz) deleted the listindent-revive branch November 10, 2018 02:21
@yume-chan Simon Chan (yume-chan) mentioned this pull request Jun 21, 2019
Extra indent added after multi-line destructured assignment #19167
Closed
@microsoft Microsoft (microsoft) locked as resolved and limited conversation to collaborators Oct 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@sheetalkamat Sheetal Nandi (sheetalkamat) sheetalkamat approved these changes
+1 more reviewer
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@sheetalkamat Sheetal Nandi (sheetalkamat)

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Indentation issues on multiline call expressions unexpected chained "then" method indentation formatting problem with an object literal as an inlined result type of a function angle brackets formatting problem fluent api style method chain formatting problem
Formatting with method parameters Revive PR #13574

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@saschanaz @DanielRosenwasser @sheetalkamat
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.