- Notifications
You must be signed in to change notification settings - Fork13.2k
Interactive type inlay hints#55141
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
typescript-bot commentedJul 24, 2023
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
91c2844 todec7b99Comparefb19ede to420610aCompareUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
src/services/inlayHints.ts Outdated
| constparts:InlayHintDisplayPart[]=[]; | ||
| visitDisplayPart(typeNode); | ||
| functionvisitDisplayPart(node:Node){ |
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.
nit: you're not visiting display parts, you're visiting nodes to aggregate display parts
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.
Yeah I just didn't want to call itvisitor to differentiate it from the one at the top ofprovideInlayHints. Suggestions of a better name?
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.
visitForDisplayParts?
| parts.push({text:">"}); | ||
| } | ||
| // TODO: Parameters. | ||
| parts.push({text:" => "}); |
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.
Instead of creating objects on the fly, I'm pretty sure we have display part functions that construct these. They have the benefit that they can include a display part kind as well if I recall correctly.
On that note, I think spaces are typically their own display part.
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.
Yeah but those are forSymbolDisplayParts, and I haveInlayHintDisplayParts here.
6980ff3 to82fb813Compare169dd7f to3fdba53CompareMariaSolOs commentedAug 17, 2023
@jakebailey should I rebase this PR to include the new formatting setup? |
1442daa to6ae5fc0CompareUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
RyanCavanaugh commentedSep 19, 2023
@jakebailey can you review the latest commit? Thanks! |
jakebailey commentedSep 19, 2023
The changes look good, I was just waiting on#55141 (comment) as that was in my review comments from the last batch. |
jakebailey left a 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.
Of course I don't really mind the name either way.
Gawd71730 commentedJan 17, 2024
Hints |
Second iteration of#47693