-
Notifications
You must be signed in to change notification settings - Fork 200
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
Cohost code actions #11147
Cohost code actions #11147
Conversation
…tionsPart5 # Conflicts: # src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/CodeActions/CodeActionsService.cs
…ine before. I blame VS
Merged main and bumped to a real Roslyn build, so this is ready for review |
@dotnet/razor-compiler Your |
Please try regenerating the baseline so we can see the diff, it's not very clear from the test results |
Spread changed to two dots? Seems odd to me, but not necessarily anything to worry about I suppose: https://github.com/dotnet/razor/pull/11147/files#diff-77d4727ed03808a308baf4eb6f7861529d0a9d23e86b2b51dda9eb9d719b62bd |
@@ -23,7 +23,8 @@ | |||
Keyword;[case]; | |||
Whitespace;[ ]; | |||
LeftBracket;[[]; | |||
CSharpOperator;[..]; | |||
Dot;[.]; | |||
Dot;[.]; |
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 like result of dotnet/roslyn#75549.
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 very good!
src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Remote/CodeActionRequestInfo.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.CodeAnalysis.Remote.Razor/CodeActions/RoslynCodeActionHelpers.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.CodeAnalysis.Remote.Razor/CodeActions/RemoteCodeActionsService.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.
This file looks super cool. 😄
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.
ehh, it's okay. I don't like that we have to just manually make sure this matches everything in the LSP server DI container.
public async Task<string?> GetFormattedNewFileContentsAsync(IProjectSnapshot projectSnapshot, Uri csharpFileUri, string newFileContent, CancellationToken cancellationToken) | ||
{ | ||
Debug.Assert(projectSnapshot is RemoteProjectSnapshot); | ||
var project = ((RemoteProjectSnapshot)projectSnapshot).Project; | ||
|
||
var document = project.AddDocument(RazorUri.GetDocumentFilePathFromUri(csharpFileUri), newFileContent); | ||
|
||
return await ExternalHandlers.CodeActions.GetFormattedNewFileContentAsync(document, cancellationToken).ConfigureAwait(false); | ||
} |
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 not creating a new async state machine.
public async Task<string?> GetFormattedNewFileContentsAsync(IProjectSnapshot projectSnapshot, Uri csharpFileUri, string newFileContent, CancellationToken cancellationToken) | |
{ | |
Debug.Assert(projectSnapshot is RemoteProjectSnapshot); | |
var project = ((RemoteProjectSnapshot)projectSnapshot).Project; | |
var document = project.AddDocument(RazorUri.GetDocumentFilePathFromUri(csharpFileUri), newFileContent); | |
return await ExternalHandlers.CodeActions.GetFormattedNewFileContentAsync(document, cancellationToken).ConfigureAwait(false); | |
} | |
public Task<string?> GetFormattedNewFileContentsAsync(IProjectSnapshot projectSnapshot, Uri csharpFileUri, string newFileContent, CancellationToken cancellationToken) | |
{ | |
Debug.Assert(projectSnapshot is RemoteProjectSnapshot); | |
var project = ((RemoteProjectSnapshot)projectSnapshot).Project; | |
var document = project.AddDocument(RazorUri.GetDocumentFilePathFromUri(csharpFileUri), newFileContent); | |
return ExternalHandlers.CodeActions.GetFormattedNewFileContentAsync(document, cancellationToken); | |
} |
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.
Okay, but I need to employ a !
because the Roslyn method returns Task<string>
, so its not pretty. Unless I'm forgetting some other way to avoid the nullability compiler warning?
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.
Ahhh... I see. Yeah, that's unfortunate. However, if the Roslyn method returns a Task<string>
, why does this return a Task<string?>
? It doesn't look to me like this method can return null. Or, is the Roslyn method not annotated correctly? Or, can this also return a Task<string>
?
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.
It's the non-cohost impl. It makes an LSP call, so can always be null.
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.
Had a shower thought, I can make this non-nullable by moving the bit that handles null into the non-cohost impl. Thank you for the brain spark :D
Lost hours to that bloody capital T
@dotnet/razor-compiler for second sign off on test baseline changes |
...oft.CodeAnalysis.Razor.Workspaces/CodeActions/Razor/ExtractToCodeBehindCodeActionResolver.cs
Outdated
Show resolved
Hide resolved
Test changes should be good to go with just one sign off. |
Fixes #10742
Needs dotnet/roslyn#75711 before it will build
Also will need to merge in main once #11141 is merged