Support Generate Method code action in Razor and C# editors#12960
Support Generate Method code action in Razor and C# editors#12960davidwengier merged 30 commits intodotnet:mainfrom
Conversation
…ngs around everywhere
…from ClientSettings
…mplify the service
…lled previously This deliberately moves the call up higher in the C# on type formatting pass, because in future when we handle more language features we'll need the formatting to happen on those edits. The using directive work that is being now is simple enough that its okay to go at the end, because we just assume no indentation etc.
I could have easily gone the other way and moved the edit mapping method to document mapping service, and gotten rid of the edit mapping service. I genuinely am 50/50 on which way is better.
… the way The skipped edits will come in handy soon, I promise
…cessing skipped edits This will also help when some new methods are mappable, and some aren't
Also moved AddCSharpLanguageFeatureChanges out to the main edit service file, and renamed the CSharpLanguageFeatures file. I originally thought that would be a good place to put support for all language features, but it would have just been way too big.
Yes, the formatting is wrong, that will be part of a subsequent commit
Yes, this uses hardcoded formatting options. Will hook that up for real next
1198089 to
b3a67f8
Compare
...or/src/Microsoft.VisualStudioCode.RazorExtension/Services/VSCodeRemoteServicesInitializer.cs
Show resolved
Hide resolved
src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Remote/RazorServices.cs
Show resolved
Hide resolved
src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/CodeActions/CodeActionResolveService.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Settings/ClientSettings.cs
Show resolved
Hide resolved
src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/DocumentMapping/RazorEditService.cs
Show resolved
Hide resolved
src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/DocumentMapping/RazorEditService.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/DocumentMapping/RazorEditService.cs
Outdated
Show resolved
Hide resolved
|
If I didn't reply to a comment, it's just because I made the code change suggested without issue. And even if I did, I don't think there was anything I didn't change anyway :) |
...azor/src/Microsoft.CodeAnalysis.Razor.Workspaces/DocumentMapping/RazorEditService_Methods.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/DocumentMapping/RazorEditService.cs
Show resolved
Hide resolved
...azor/src/Microsoft.CodeAnalysis.Razor.Workspaces/DocumentMapping/RazorEditService_Methods.cs
Show resolved
Hide resolved
src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/DocumentMapping/RazorEditService.cs
Show resolved
Hide resolved
|
This looks great, optimistically approving. Thanks! |
| Start = hostEndIndex, | ||
| Length = 0 | ||
| }, | ||
| NewText = " " + Environment.NewLine + indent + newText |
There was a problem hiding this comment.
Good question. I didn't really look at the contents of this method much, just moved it, but I'll see if we have test coverage and what this is doing.
There was a problem hiding this comment.
So none of our current tests hit this line, and I can't seem to construct one that will. However since the generate methods tests are currently skipped, I'm leaning towards leaving it in place until they can be unskipped, and then checking again. If the code is dead, then an extra string literal that doesn't get hit doesn't matter, and if not, then we'll find out soon enough. I'll make a note to follow up.
There was a problem hiding this comment.
Okay, I dug into an old branch and none of this code is ever hit (ie, the whole if (!mappedStart && !mappedEnd.. branch), and the scenario that it is exclusively written for is still working. I'm going to remove it in an upcoming cleanup PR, like I talked about this morning.
|
Crap, hit merge and then realised I hadn't pushed the last few commits of follow up. Opened #12972 |
Follow ups to #12960 I spent so long trying to construct a test for #12960 (comment) that I ended up just replying to the comment, and then clicking merge.... before committing the last few commits of PR feedback 🤦♂️
Alternative to #82973 Part of dotnet/razor#6159 and dotnet/razor#4539 Razor is adding support for Generate Method (dotnet/razor#12960) but to do that we need Roslyn to offer the code action, so that's what this does. ###### Microsoft Reviewers: [Open in CodeFlow](https://microsoft.github.io/open-pr/?codeflow=https://github.com/dotnet/roslyn/pull/83028)
Part of #6159 and #4539
Reviewing commit and a time thoroughly recommended. It's a big PR, but each commit should be easy enough to understand and review. Also, for the commits that are just moving methods around files, there aren't any changes to the methods themselves.
This includes #12956 which I closed because CI wasn't starting and GitHub was missing a commit in that PR anyway. Sorry!
Tests are all passing locally, but some are skipped in this PR because we need a Roslyn bump for them to pass, to something containing dotnet/roslyn#82973. I want to get this PR in first though, so that when Roslyn starts offering Generate Method in Razor files, we're ready to handle it. In the meantime, the RazorEditServiceTests provide good coverage of the important parts of the new code, as well as all of the existing tests for all of the code moves.