-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Expose 5 more things to Razor cohosting #73781
Conversation
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.
Looking pretty good to me!
src/Features/LanguageServer/Protocol/Handler/Definitions/AbstractGoToDefinitionHandler.cs
Outdated
Show resolved
Hide resolved
src/Features/LanguageServer/Protocol/Handler/Rename/PrepareRenameHandler.cs
Outdated
Show resolved
Hide resolved
{ | ||
public static Task<DocumentHighlight[]?> GetHighlightsAsync(Document document, LinePosition linePosition, CancellationToken cancellationToken) | ||
{ | ||
var globalOptions = document.Project.Solution.Services.ExportProvider.GetExports<IGlobalOptionService>().First().Value; |
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.
Consider adding GetFirstExport<T>()
and GetExports<T>()
helper extension methods to clean up all these calls to document.Project.Solution.Services.ExportProvider.GetExports<T>()
.
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.
Great idea. I called it GetService
so it looks more normal :)
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.
That works, Although, I was actually thinking that the extension method would target Document
, so you could write document.GetExport<IGlobalOptionService>()
and save a lot of dotting. I'm not sure I agree that GetService
looks all that normal when called off of an ExportProvider
, where GetExport
is the standard API.
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.
I'm happy to change it if thats the standard API, but in my mind GetExport would give me the Lazy, and GetService the actual impl.
Also, I know this is super constrained to Razor EA, but I think an extension on Document might be confusing because Roslyn has document services language services, workspace services etc. and this is specifically intended for broader MEF services, not Roslyn MEF. If that makes any sense.
Ping @dibarbet @CyrusNajmabadi and/or @dotnet/roslyn-ide for reviews please :) |
Allows Razor to call Go To Def, OnAutoInsert, Document Highlight, Rename and Signature Help
Please pay close attention to the 4th last and 2nd last commit, as they are necessary for the code to run in OOP, where accessing
Solution.Workspace
is not allowed/possible. If you have better ideas, let me know (particularly with regards to extended language services. It seems like the move away from workspace services andProject.LanguageServices
was designed to stop exposing access to a workspace inadvertently, but "extended language services" seems to do that, even though its the desired replacement??)Also if there is a more preferred pattern than just exposing static methods from the handlers. It seems innocent enough, but now we're racking up a few of them, perhaps you prefer something better/different?