Pre co-hosting Hover tweaks #11131
Pre co-hosting Hover tweaks #11131
Conversation
The document file path can be retrieved from the RazorCodeDocument parameter.
| documentFilePath, binding.Descriptors, span, options, solutionQueryOperations, cancellationToken); | ||
| codeDocument.Source.FilePath, binding.Descriptors, span, options, solutionQueryOperations, cancellationToken); |
There was a problem hiding this comment.
Warning: Thoughts ahead!
I don't think I realised that a file path was attainable from a RazorCodeDocument, and now I'm wondering if there are other places where I've pass in an IDocumentSnapshot that could actually have taken a RazorCodeDocument.
But I'm also wondering the opposite. Presumably this codeDocument came from an IDocumentSnapshot in the caller, so would it make sense to pass an IDocumentSnapshot all the way through to the GetProjectAvailability methods? In LSP it wouldn't change much, but perhaps it would allow for smarter code in Cohosting, is there if an existing Roslyn API to go from a TextDocument to all of the projects it is contained in. Though I don't think that fits with the current solution query API, so probably unnecessarily overcomplicated for now.
There was a problem hiding this comment.
There comes a point where it bumps into completion needing tooltips as well. So, at most we would have only been able to push the RazorCodeDocument one method call further. Once everything's happily in co-hosting, I think that's a good change to make.
| { | ||
| descriptionBuilder.AppendLine(); | ||
| descriptionBuilder.Append(availability); | ||
| var availability = await solutionQueryOperations |
There was a problem hiding this comment.
❓ reading this I would have expected ISolutionQueryOperations.GetProjectAvailabilityTextAsync to handle a null file path and return null. Is there a design decision why it doesn't?
There was a problem hiding this comment.
Good question! The reason is primarily because there's a cost associated with calling GetProjectAvailabilityTextAsync(...). If the null check happens in GetProjectAvailabilityTextAsync(...) we have to make an async call and continue on the thread pool just to perform a null check. That seemed unnecessarily wasteful to me.
Another more opinionated meta reason is that the contract of GetProjectAvailabilityTextAsync(...) is to take a document file path and do something with it. By using a non-nullable string the API states its intent and makes it clear that it needs a document file path to function. By making it nullable, I think the contract is weaker. That might just be my opinion, but non-nullable reference types are all about stating intent, and I don't think GetProjectAvailabilityTextAsync(...) ever wants to be passed a null document file path, so it probably should be clear about that. At least, that was my thought process.
This pull request makes a couple of changes to
HoverFactory.GetHoverAsync(...)to prepare for co-hosting:HoverFactory.GetHoverAsync(...)returns an LSPHoverrather thanVSInternalHover.HoverFactory.GetHoverAsync(...)no longer takes a document file path. It already takes aRazorCodeDocument, and the file path can be retrieved from that.