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.

Sign up for GitHub

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

Jump to bottom

Interactive type inlay hints #55141

Merged
merged 4 commits into from Sep 20, 2023
Merged

Interactive type inlay hints #55141

merged 4 commits into from Sep 20, 2023

Conversation

MariaSolOs
Copy link
Member

Second iteration of #47693

@typescript-bot
Copy link
Collaborator

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Jul 24, 2023
@MariaSolOs MariaSolOs marked this pull request as draft July 24, 2023 21:12
@MariaSolOs MariaSolOs force-pushed the type-hints branch 3 times, most recently from 91c2844 to dec7b99 Compare August 4, 2023 01:49
@MariaSolOs MariaSolOs force-pushed the type-hints branch 2 times, most recently from fb19ede to 420610a Compare August 4, 2023 22:21
DanielRosenwasser
DanielRosenwasser reviewed Aug 5, 2023
View reviewed changes
src/services/inlayHints.ts Outdated

const parts: InlayHintDisplayPart[] = [];
visitDisplayPart(typeNode);
function visitDisplayPart(node: Node) {
Copy link
Member

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

Copy link
Member Author

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?

Copy link
Member

Choose a reason for hiding this comment

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

visitForDisplayParts?

DanielRosenwasser
DanielRosenwasser reviewed Aug 5, 2023
View reviewed changes
src/services/inlayHints.ts
parts.push({ text: ">" });
}
// TODO: Parameters.
parts.push({ text: " => " });
Copy link
Member

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.

Copy link
Member Author

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 SymbolDisplayParts, and I have InlayHintDisplayParts here.

@MariaSolOs MariaSolOs force-pushed the type-hints branch 5 times, most recently from 6980ff3 to 82fb813 Compare August 10, 2023 02:48
@MariaSolOs MariaSolOs mentioned this pull request Aug 10, 2023
Merged
@MariaSolOs MariaSolOs force-pushed the type-hints branch 3 times, most recently from 169dd7f to 3fdba53 Compare August 17, 2023 01:53
@MariaSolOs
Copy link
Member Author

@jakebailey should I rebase this PR to include the new formatting setup?

@MariaSolOs MariaSolOs mentioned this pull request Aug 17, 2023
Merged
@sandersn sandersn added this to Not started in PR Backlog Aug 18, 2023
@sandersn sandersn moved this from Not started to Waiting on reviewers in PR Backlog Aug 23, 2023
@MariaSolOs MariaSolOs mentioned this pull request Sep 1, 2023
Open
@RyanCavanaugh
Copy link
Member

@jakebailey can you review the latest commit? Thanks!

@jakebailey
Copy link
Member

The changes look good, I was just waiting on #55141 (comment) as that was in my review comments from the last batch.

jakebailey
jakebailey approved these changes Sep 20, 2023
View reviewed changes
Copy link
Member

@jakebailey jakebailey left a 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.

PR Backlog automation moved this from Waiting on reviewers to Needs merge Sep 20, 2023
@MariaSolOs MariaSolOs merged commit 0b82e1a into microsoft:main Sep 20, 2023
19 checks passed
PR Backlog automation moved this from Needs merge to Done Sep 20, 2023
@MariaSolOs MariaSolOs deleted the type-hints branch September 20, 2023 05:50
snovader pushed a commit to mestro-se/TypeScript that referenced this pull request Sep 23, 2023
@MariaSolOs @snovader
Interactive type inlay hints (microsoft#55141)
7f455f5
@ha1fstack ha1fstack mentioned this pull request Nov 26, 2023
Open
@luabud luabud mentioned this pull request Dec 4, 2023
Open
@Gawd71730
Copy link

Hints

@MariaSolOs MariaSolOs mentioned this pull request Apr 14, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DanielRosenwasser DanielRosenwasser DanielRosenwasser left review comments

@jakebailey jakebailey jakebailey approved these changes

@zkat zkat Awaiting requested review from zkat

@joj joj Awaiting requested review from joj

@andrewbranch andrewbranch Awaiting requested review from andrewbranch

@RyanCavanaugh RyanCavanaugh Awaiting requested review from RyanCavanaugh

@gabritto gabritto Awaiting requested review from gabritto

@sheetalkamat sheetalkamat Awaiting requested review from sheetalkamat

Assignees

@MariaSolOs MariaSolOs

Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
PR Backlog
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@MariaSolOs @typescript-bot @jakebailey @RyanCavanaugh @Gawd71730 @DanielRosenwasser