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

Support inlay hints #5107

Merged
merged 12 commits into from
Mar 29, 2022
Merged

Support inlay hints #5107

merged 12 commits into from
Mar 29, 2022

Conversation

333fred
Copy link
Member

@333fred 333fred commented Mar 16, 2022

Closes OmniSharp/omnisharp-roslyn#1932. Implements the new inlay hint api for vscode, now that it's stable and in LSP. Tests still need to be written.

@333fred
Copy link
Member Author

333fred commented Mar 16, 2022

This will need OmniSharp/omnisharp-roslyn#2357 to be merged, and I still need to write some integration tests covering the new feature. Some screenshots:

image
image

@somedeveloperhappy
Copy link

I just wanted to come here and thank you guys for all your efforts. the inlay hints was one of the main reasons I still have Rider installed on my machine

Copy link
Member

@JoeRobich JoeRobich left a comment

Choose a reason for hiding this comment

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

Did you test this with a Razor scenario?

Do we need to filter out Razor generated documents?
https://github.com/OmniSharp/omnisharp-vscode/blob/84f1d5ab16718c2e0ad91e732770515500a8113e/src/features/virtualDocumentTracker.ts#L25-L39

Or handle mapping location information to Razor documents by passing it through the LanguageMiddleware?
https://github.com/OmniSharp/omnisharp-vscode/blob/84f1d5ab16718c2e0ad91e732770515500a8113e/src/features/referenceProvider.ts#L25-L27

src/omnisharp/options.ts Outdated Show resolved Hide resolved
test/unitTests/Fakes/FakeOptions.ts Show resolved Hide resolved
333fred added 2 commits March 24, 2022 19:53
Closes OmniSharp/omnisharp-roslyn#1932. Implements the new inlay hint api for vscode, now that it's stable and in LSP. Tests still need to be written.
@333fred
Copy link
Member Author

333fred commented Mar 25, 2022

I think we should probably avoid razor documents for now, until @NTaylorMullen has a chance to test it out and make sure that everything is working as expected.

@333fred 333fred marked this pull request as ready for review March 25, 2022 03:06
@333fred 333fred force-pushed the inlay-hints branch 7 times, most recently from aeb6ebd to 6141d88 Compare March 25, 2022 06:28
Copy link
Member

@JoeRobich JoeRobich left a comment

Choose a reason for hiding this comment

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

Suggestion on how to make the test more consistent

test/integrationTests/inlayHints.integration.test.ts Outdated Show resolved Hide resolved
test/integrationTests/inlayHints.integration.test.ts Outdated Show resolved Hide resolved
@somedeveloperhappy
Copy link

hey. I really need this feature even if it's in beta. can someone help me on how to use this branch in vscode? I could be helpful with my bug reports later :p

@333fred
Copy link
Member Author

333fred commented Mar 29, 2022

@JoeRobich we'll see, but locally the test doesn't even require that the file is open at all. Something else is happening.

@333fred
Copy link
Member Author

333fred commented Mar 29, 2022

hey. I really need this feature even if it's in beta. can someone help me on how to use this branch in vscode? I could be helpful with my bug reports later :p

Check out the branch and build it.

@somedeveloperhappy
Copy link

somedeveloperhappy commented Mar 29, 2022

hey. I really need this feature even if it's in beta. can someone help me on how to use this branch in vscode? I could be helpful with my bug reports later :p

Check out the branch and build it.

yes, trying. but my npm gives out too many errors :(
is there some magic command that installs all needed npm packages?

  • also if you don't have time, I'll still be very glad if you at least tell me what to search/google about that

Nevermind. I successfully built the package and tested it and THANKS! I wish I could somehow support you guys because this feature made me completely delete Rider and switch to vscode for good :)

@nohwnd nohwnd added the Triaged label Mar 29, 2022
333fred and others added 4 commits March 29, 2022 11:58
Co-authored-by: Joey Robichaud <joseph.robichaud@microsoft.com>
Co-authored-by: Joey Robichaud <joseph.robichaud@microsoft.com>
@333fred 333fred merged commit 7689b0d into dotnet:master Mar 29, 2022
@333fred 333fred deleted the inlay-hints branch March 29, 2022 23:57
@somedeveloperhappy
Copy link

Hey, umm , this is written "Merged" does it mean It's officially gonna go to the master branch now? Or just to the master of this fork?

Sorry I don't know much about GitHub

@333fred
Copy link
Member Author

333fred commented Mar 31, 2022

This isn't a fork, it's the main repository. And yes, it has been merged to master and will be in the next release of omnisharp vscode.

@somedeveloperhappy
Copy link

Thanks, yeah, figured that out by some research afterwards :D thanks. And the feature is really great! You guys are breaking the boundaries between vscode and Rider for dotnet!

  • Though the new omnisharp loads suggestions after maybe a whole 1 second :( I'm assuming it's got something to do with the omnisharp itself and not the inlay hints, but just wanted to let you know

@JoeRobich
Copy link
Member

@somedeveloperhappy Do you have either of these options enabled? Async completion should help with completion performance.

    "omnisharp.enableAsyncCompletion": true,
    "omnisharp.enableImportCompletion": true,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for Inline Parameter Name Hints
5 participants