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

Lookup Razor file context using its document uri. #7663

Merged
merged 2 commits into from
Oct 16, 2024

Conversation

JoeRobich
Copy link
Member

Previously we had been looking up Razor document context from its mapped C# file. This is unnecessary as we can look it up directly from the razor file uri.

@JoeRobich JoeRobich requested a review from a team as a code owner October 15, 2024 20:54
@davidwengier
Copy link
Contributor

This is unnecessary

Only if the .razor and .cshtml files are in the Roslyn workspace presumably, which is not 100% of projects that have Razor files in them. Not sure if you care though :)

A possible alternative would be to send the request to the Razor server, but I'm not sure what you do with the project context response, and it will not match Roslyn project Ids etc.

@ryzngard
Copy link
Contributor

Only if the .razor and .cshtml files are in the Roslyn workspace presumably

That would mean that this warning is still potentially correct. As an aside working in the razor repo with VS Code is a bit weird because we have test razor files that have errors...

@JoeRobich
Copy link
Member Author

I'm not sure what you do with the project context response, and it will not match Roslyn project Ids etc.

We display the context name in the language status bar as the active file context. Longer term we would want to look at using the returned project contexts in a picker to choose the active file context. In that future, TextDocumentIdentifiers in requests to Roslyn LSP will be updated with the selected active project context.

@davidwengier
Copy link
Contributor

That would mean that this warning is still potentially correct.

"this warning"? I have no context on what this change is fixing, so I can't really say if this is better or worse than before 😛

TextDocumentIdentifiers in requests to Roslyn LSP will be updated with the selected active project context

Until cohosting is a thing, there presumably won't be any requests to the Roslyn LSP for Razor documents. Right now its a little odd not asking Razor for project contexts when it is the server that will be sent subsequent requests, but if those requests don't have project contexts then it doesn't matter which contexts the requests don't have 🤷‍♂️

TL;DR: I have no idea if this is a good change or a bad change, but the stakes seem pretty low at the moment so I wouldn't block anything on my comments.

@davidwengier
Copy link
Contributor

I looked at the bug this is fixing, and it seems to me the most appropriate thing here would be to just remove "aspnetcorerazor" from the language checks. The fact is that not all language features are available in Razor files, and since Roslyn isn't the one providing them anyway, it seems odd that it would show any kind of warning at all.

@JoeRobich
Copy link
Member Author

it seems odd that it would show any kind of warning at all

Updated to not show warning toasts for Razor files.

@JoeRobich JoeRobich merged commit d8c096e into main Oct 16, 2024
16 checks passed
@JoeRobich JoeRobich deleted the dev/jorobich/razor-doc-context branch October 17, 2024 00:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants