Upgrade OmniSharp to 0.19.4. #4134
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- With the latest OmniSharp upgrade there were a few high profile changes:
- Most DTO's are records now which makes them immutable. You'll see throughout the codebase that we now have to use `with {...}` to translate one type to another.
- `ServerCapabilities` are expandable by default. This means we no longer need our `ExtendableServerCapabilities` type.
- Getting client and server capabilities in each of our endpoints are now a single method call. This resulted in lots of deleted code.
- O# upgraded its LSP version to 3.17 which means semantic tokens are no longer proposed. This resulted in a lot of warnings/obsolete bits getting removed. We now also have code action resolution as part of this upgrade so we could remove our old code action resolution endpoint (it's in VSCode now too).
- The way the O# serializer gets construed now is different and extendable. Because of this we now have a primary method to add all of our converters to an O# serializer.
- O# embraced the optional vs. required text document identifiers. This makes it super clear whenever we're expected to provided a document version or not. Probably one of my favorite changes in the upgrade.
- A new dependency of `System.Threading.Channels` was introduced so we had to make sure that was included in our VS scenarios.
- This changeset is in preparation for another O# release where we'll replace the [O# request invoker](OmniSharp/csharp-language-server-protocol#641) to get some pretty massive perf wins!
Fixes dotnet/aspnetcore#35622
NTaylorMullen
requested review from
a team,
allisonchou,
ryanbrandenburg and
sharwell
as code owners
davidwengier
approved these changes
Aug 24, 2021
Member
davidwengier
left a comment
davidwengier
left a comment
There was a problem hiding this comment.
LGTM
Out of curiosity, is this version of O# nullable annotated?
...Core.Razor.LanguageServer/CodeActions/CSharp/UnformattedRemappingCSharpCodeActionResolver.cs
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Completion/RazorCompletionEndpoint.cs
Show resolved
Hide resolved
...Microsoft.AspNetCore.Razor.LanguageServer/CodeActions/Models/OmniSharpVSCodeActionContext.cs
Outdated
Show resolved
Hide resolved
...Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Serialization/LspSerializerExtensions.cs
Outdated
Show resolved
Hide resolved
ryanbrandenburg
approved these changes
Aug 24, 2021
Contributor
ryanbrandenburg
left a comment
ryanbrandenburg
left a comment
There was a problem hiding this comment.
Looks good. Some small quibbles, but mostly want to be sure that we've run it through the paces in the experimental instance. If I recall last time there were some problems which didn't show in our tests, only in functional usage.
src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Completion/RazorCompletionEndpoint.cs
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/RazorLanguageServer.cs
Show resolved
Hide resolved
...r/src/Microsoft.AspNetCore.Razor.LanguageServer/Semantic/Models/RazorSemanticTokensLegend.cs
Outdated
Show resolved
Hide resolved
...Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Serialization/LspSerializerExtensions.cs
Outdated
Show resolved
Hide resolved
Author
Yes! |
- Set => Init - Removal of semantic token legend endpoint - Some refactoring in our serializer extensions - Test fixups
Author
|
🆙 📅 If all looks good i'll get in. @allisonchou looks like latest O# also has a linked editing range handler too 😄
|
Contributor
|
@NTaylorMullen perfect! Then I can get rid of the extra classes I added 😄 |
NTaylorMullen
added
the
auto-merge
|
Hello @NTaylorMullen! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
ghost
merged commit
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
with {...}to translate one type to another.ServerCapabilitiesare expandable by default. This means we no longer need ourExtendableServerCapabilitiestype.System.Threading.Channelswas introduced so we had to make sure that was included in our VS scenarios.Fixes dotnet/aspnetcore#35622
/cc @ToddGrun @jimmylewis