-
Notifications
You must be signed in to change notification settings - Fork 12.3k
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
Interactive type inlay hints #55141
Interactive type inlay hints #55141
Conversation
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
91c2844
to
dec7b99
Compare
fb19ede
to
420610a
Compare
src/services/inlayHints.ts
Outdated
|
||
const parts: InlayHintDisplayPart[] = []; | ||
visitDisplayPart(typeNode); | ||
function visitDisplayPart(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 it visitor
to differentiate it from the one at the top of provideInlayHints
. 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 for SymbolDisplayPart
s, and I have InlayHintDisplayPart
s here.
6980ff3
to
82fb813
Compare
169dd7f
to
3fdba53
Compare
@jakebailey should I rebase this PR to include the new formatting setup? |
119a2d6
to
0497a33
Compare
@jakebailey can you review the latest commit? Thanks! |
The changes look good, I was just waiting on #55141 (comment) as that was in my review comments from the last batch. |
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.
Hints |
Second iteration of #47693