Enable extract refactorings in LSP #76718
Conversation
dotnet-issue-labeler
bot
added
Area-Infrastructure
untriaged
JoeRobich
force-pushed
the
dev/jorobich/enable-extract-refactorings
branch
from
5399edf to
b4582d8
Compare
| UnifiedPredefinedSuggestedActionCategoryNames.StyleFix => CodeActionKind.QuickFix, | ||
| UnifiedPredefinedSuggestedActionCategoryNames.ErrorFix => CodeActionKind.QuickFix, | ||
| _ => throw ExceptionUtilities.UnexpectedValue(categoryName) | ||
| }; | ||
|
|
||
| private static CodeActionKind GetRefactoringKind(IUnifiedSuggestedAction suggestedAction) |
There was a problem hiding this comment.
Providing the proper type will have VS Code render the actions differently.
| ReferenceOutputAssembly="false" | ||
| Private="false" /> | ||
| <ItemGroup> | ||
| <PackageReference Include="Microsoft.CodeAnalysis.CSharp.Analyzer.Testing.XUnit" /> |
There was a problem hiding this comment.
This is added to parse test code markup.
JoeRobich
force-pushed
the
dev/jorobich/enable-extract-refactorings
branch
from
b4582d8 to
51e0c13
Compare
src/LanguageServer/Protocol/Handler/CodeActions/CodeActionHelpers.cs
Outdated
Show resolved
Hide resolved
src/LanguageServer/Protocol/Handler/CodeActions/CodeActionHelpers.cs
Outdated
Show resolved
Hide resolved
...Analysis.LanguageServer.UnitTests/Utilities/AbstractLanguageServerHostTests.TestLspServer.cs
Outdated
Show resolved
Hide resolved
JoeRobich
force-pushed
the
dev/jorobich/enable-extract-refactorings
branch
from
a97921b to
59a55dc
Compare
| <!-- | ||
| Copy files contained in the project to a RoslynLSP subdirectory to emulate deployment of the language server. | ||
| --> | ||
| <Target Name="_CopyLanguageServerFiles" AfterTargets="ResolveProjectReferences"> |
There was a problem hiding this comment.
We need the LanguageServer isolated into its own folder. This target works similarly to the already preset _CopyDevKitExtensionFiles.
...ageServer/Microsoft.CodeAnalysis.LanguageServer/Microsoft.CodeAnalysis.LanguageServer.csproj
Outdated
Show resolved
Hide resolved
JoeRobich
changed the title
| Copy files contained in the project to a RoslynLSP subdirectory to emulate deployment of the language server. | ||
| --> | ||
| <Target Name="_CopyLanguageServerFiles" AfterTargets="ResolveProjectReferences"> | ||
| <MSBuild Projects="..\Microsoft.CodeAnalysis.LanguageServer\Microsoft.CodeAnalysis.LanguageServer.csproj" Targets="GetOutputPath"> |
There was a problem hiding this comment.
Definitely test this in VS to make sure this doesn't cause issues with the VS fast up todate check (basically a build, then build again with no changes shouldn't rebuild any of these projects)
There was a problem hiding this comment.
I think we are good.
Build started at 3:14 PM...
========== Build: 0 succeeded, 0 failed, 23 up-to-date, 0 skipped ==========
========== Build completed at 3:14 PM and took 00.852 seconds ==========
| _logger.LogError(e, "Exception while shutting down the LSP process."); | ||
|
|
||
| // Process may have gone bad, so not much else we can do. | ||
| _process.Kill(); |
There was a problem hiding this comment.
Wondering if we should have an assert here after this is killed to fail the test - unless we have tests specifically testing a failed shutdown we shouldn't hit this
|
|
||
| <ItemGroup Condition="'$(_LanguageServerOutputPath)' != ''"> | ||
| <_LanguageServerFile Include="$(_LanguageServerOutputPath)\**\*.*" /> | ||
| <_LanguageServerFile Remove="$(_LanguageServerOutputPath)\**\*.mef-composition" /> |
There was a problem hiding this comment.
this deletes the mef cache right? wonder if we should delete this every time the test runs (instead of in msbuild)
There was a problem hiding this comment.
This is mostly for local dev scenarios where many compositions may exist in your local build of Microsoft.CodeAnalysis.LangaugeServer and there is no need to move them to the test folder. Lets save deleting compositions on each test run for when we move the ExportProviderBuilder tests to use the LSP.
...crosoft.CodeAnalysis.LanguageServer.UnitTests/Utilities/AbstractLanguageServerClientTests.cs
Outdated
Show resolved
Hide resolved
|
|
||
| namespace Microsoft.CodeAnalysis.LanguageServer.UnitTests; | ||
|
|
||
| internal interface ILspClient |
There was a problem hiding this comment.
is there much of a point in having this? the test needs the actual lsp server as it needs to dispose of it
There was a problem hiding this comment.
The benefit here is that TestLspServer and TestLspClient can use the same set of extension methods to drive the language server.
|
Merging, will take any additional feedback as a follow up. |
Options services are added to the LanguageServer project so that the extract refactorings will run. The options use defaults and do not require user interaction. This is part of the O# parity work and the defaults match those used by O#.
Once inserted, this will resolve dotnet/vscode-csharp#6430