-
Notifications
You must be signed in to change notification settings - Fork 199
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
VS Razor Code Actions #2420
VS Razor Code Actions #2420
Conversation
src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/CodeActions/Models/RazorCodeAction.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/CodeActions/Models/RazorCodeAction.cs
Show resolved
Hide resolved
Love how the preview is available for a lot of the code actions. I'd reach out to the LSP platform team to see why they didn't provide the "preview" support for edits that were already available on the CodeAction |
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.
Looks solid! Exactly how I was thinking it'd fit together. Man it's exciting to see this land 👏
...zor/src/Microsoft.AspNetCore.Razor.LanguageServer/CodeActions/AddUsingsCodeActionResolver.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/CodeActions/CodeActionEndpoint.cs
Outdated
Show resolved
Hide resolved
...rc/Microsoft.AspNetCore.Razor.LanguageServer/CodeActions/Models/RazorCodeActionExtensions.cs
Outdated
Show resolved
Hide resolved
...r/src/Microsoft.AspNetCore.Razor.LanguageServer/CodeActions/ILSPCodeActionResolverHandler.cs
Outdated
Show resolved
Hide resolved
...or/src/Microsoft.AspNetCore.Razor.LanguageServer/CodeActions/CodeActionResolutionEndpoint.cs
Outdated
Show resolved
Hide resolved
...soft.AspNetCore.Razor.LanguageServer/CodeActions/ComponentAccessibilityCodeActionProvider.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/CodeActions/Models/RazorCodeAction.cs
Outdated
Show resolved
Hide resolved
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.
Looks great! Mostly minor comments
Also, would be good to make sure the changes here didn't accidentally regress VSCode.
src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/CodeActions/CodeActionEndpoint.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/CodeActions/CodeActionEndpoint.cs
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/CodeActions/CodeActionEndpoint.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/CodeActions/CodeActionEndpoint.cs
Outdated
Show resolved
Hide resolved
...soft.AspNetCore.Razor.LanguageServer/CodeActions/ComponentAccessibilityCodeActionProvider.cs
Outdated
Show resolved
Hide resolved
...c/Microsoft.AspNetCore.Razor.LanguageServer/CodeActions/CreateComponentCodeActionResolver.cs
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/CodeActions/Models/RazorCodeAction.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/CodeActions/Models/RazorCodeAction.cs
Show resolved
Hide resolved
Updated with PR feedback. @NTaylorMullen suggested removing the existing
Yep, validated all four in VSCode 💯 |
src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/CodeActions/CodeActionEndpoint.cs
Outdated
Show resolved
Hide resolved
...or/src/Microsoft.AspNetCore.Razor.LanguageServer/CodeActions/CodeActionResolutionEndpoint.cs
Outdated
Show resolved
Hide resolved
...crosoft.AspNetCore.Razor.LanguageServer/CodeActions/ExtractToCodeBehindCodeActionProvider.cs
Outdated
Show resolved
Hide resolved
...rc/Microsoft.AspNetCore.Razor.LanguageServer/CodeActions/Models/RazorCodeActionExtensions.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.VSCode/src/CodeActions/RazorCodeActionRunner.ts
Show resolved
Hide resolved
Ahhh one other note I forgot to mention that can totally be done in a follow up. We'll need to disable the file operation based CodeActions in LiveShare guest/Codespaces scenarios until they make them work. That's because the underlying system will pre-create the file based off the existence of the light bulb. Meaning when a user is typing This alone may be a difficult problem to handle. Some ideas would be to implement a MiddleLayer (assuming the LSP platform supports them for CodeActions) or somehow utilize the |
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.
Looks good, just curious about a couple things.
public CodeActionEndpoint( | ||
IEnumerable<RazorCodeActionProvider> providers, | ||
ForegroundDispatcher foregroundDispatcher, | ||
DocumentResolver documentResolver, | ||
ILoggerFactory loggerFactory) | ||
ILanguageServer languageServer) |
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.
Could you make this Lazy<ILanguageServer>
to future proof it against O# upgrade? In new version of O# there's a possibility of a DI infinite-loop (O# is working on a fix last I heard).
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.
Hmm, so I just tried that System.Lazy<ILanguageServer>
, but it results in the endpoint never being constructed in the first place.
It seems in the current version of O#, ILanguageServer
is already available, so perhaps the Lazy<ILanguageServer>
isn't recognized?
@@ -75,13 +105,23 @@ public async Task<CommandOrCodeActionContainer> Handle(CodeActionParams request, | |||
return null; | |||
} | |||
|
|||
if (cancellationToken.IsCancellationRequested) |
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.
How did you determine where it was appropriate to put cancellation checks? Obviously once at the top is likely to be insufficient, but maybe before every async call is too often? I have little experience with this, so mostly for my own education.
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.
My reasoning was certain functionality like code actions, hover & completions are likely to be triggered very frequently (just by moving a mouse / typing a character), and subsequently dismissed / cancelled frequently (when the mouse is moved further along the path / next character is typed).
For this type of functionality, I think we should be a bit more aggressive in our cancellation checks, which is what I've tried doing here.
Created: https://github.com/dotnet/aspnetcore/issues/25128 & pulled into current sprint.
😱 that doesn't sound too good. |
Summary of the changes
Fixes: https://github.com/dotnet/aspnetcore/issues/23985