Skip to content

upgraded to omnisharp 19 #1802

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
May 25, 2021
Merged

upgraded to omnisharp 19 #1802

merged 3 commits into from
May 25, 2021

Conversation

majastrz
Copy link
Member

Upgraded the language server and test projects to use OmniSharp 19. This brings full support of LSP 3.16, including the semantic token legend, which should allow more control over syntax highlighting via semantic tokens.

OmniSharp release notes: https://github.com/OmniSharp/csharp-language-server-protocol/releases/tag/v0.19.0

@majastrz majastrz added this to the v0.4 milestone Mar 10, 2021
majastrz
majastrz commented Mar 10, 2021
View reviewed changes
src/Bicep.LangServer/SemanticTokenVisitor.cs
@@ -106,7 +105,7 @@ public override void VisitObjectPropertySyntax(ObjectPropertySyntax syntax)
}
else
{
AddTokenType(syntax.Key, SemanticTokenType.Member);
AddTokenType(syntax.Key, SemanticTokenType.Method);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Method [](start = 59, length = 6)

Member was renamed to Method.

majastrz
majastrz commented Mar 10, 2021
View reviewed changes
src/Bicep.LangServer/Completions/CompletionItemBuilder.cs
// Licensed under the MIT License.
using System;
using OmniSharp.Extensions.LanguageServer.Protocol.Models;
using Range = OmniSharp.Extensions.LanguageServer.Protocol.Models.Range;

namespace Bicep.LanguageServer.Completions
{
public static class CompletionItemBuilder
public class CompletionItemBuilder
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CompletionItemBuilder [](start = 17, length = 21)

CompletionItem is now a record, which is immutable.

majastrz
majastrz commented Mar 10, 2021
View reviewed changes
src/Bicep.LangServer/Handlers/BicepTextDocumentSyncHandler.cs
=> new TextDocumentSaveRegistrationOptions
{
DocumentSelector = DocumentSelectorFactory.Create(),
IncludeText = true,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IncludeText [](start = 16, length = 11)

There doesn't appear to be an equivalent for IncludeText, but nothing appears to be broken.

majastrz
majastrz commented Mar 10, 2021
View reviewed changes
src/Bicep.LangServer.IntegrationTests/Helpers/IntegrationTestHelper.cs Outdated
using Bicep.Core.Navigation;

namespace Bicep.LangServer.IntegrationTests
{
public static class IntegrationTestHelper
{
private const int DefaultTimeout = 20000;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

20000 [](start = 43, length = 5)

When running all the tests in parallel in VS, I would occasionally get a timeout that goes away on retry. Increased it to 20s from 10s.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not applicable anymore.

majastrz
majastrz commented Mar 10, 2021
View reviewed changes
src/Bicep.LangServer.UnitTests/BicepCompletionProviderTests.cs Outdated
@@ -72,7 +72,7 @@ public void DeclarationSnippetsShouldBeValid()
foreach (var (detail, completion) in snippetsByDetail)
{
// validate snippet
var snippet = new Snippet(completion.TextEdit!.NewText);
var snippet = new Snippet(completion.TextEdit!.TextEdit!.NewText);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TextEdit [](start = 63, length = 8)

This fun is due to the introduction of InsertReplaceTextEdit. I haven't played with it yet, though.

@majastrz majastrz force-pushed the majastrz/omnisharp-19 branch 2 times, most recently from bb10e29 to f763508 Compare March 17, 2021 02:05
@codecov-io
Copy link

codecov-io commented Mar 17, 2021 •
edited
Loading

Codecov Report

Merging #1802 (f763508) into main (1bd5ffc) will increase coverage by 0.49%.
The diff coverage is 94.80%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1802      +/-   ##
==========================================
+ Coverage   95.30%   95.79%   +0.49%     
==========================================
  Files         377      375       -2     
  Lines       22696    23130     +434     
  Branches       15        0      -15     
==========================================
+ Hits        21630    22157     +527     
+ Misses       1066      973      -93
Flag Coverage Δ
dotnet 95.79% <94.80%> (+0.06%) ⬆️
typescript ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
....IntegrationTests/Helpers/IntegrationTestHelper.cs 96.55% <ø> (ø)
src/Bicep.LangServer/SemanticTokenVisitor.cs 0.00% <0.00%> (ø)
....LangServer/Handlers/BicepDocumentSymbolHandler.cs 83.78% <80.00%> (ø)
...p.LangServer/Handlers/BicepSignatureHelpHandler.cs 98.21% <88.88%> (-0.87%) ⬇️
....LangServer/Handlers/BicepSemanticTokensHandler.cs 75.00% <90.00%> (+1.31%) ⬆️
...ep.LangServer/Completions/CompletionItemBuilder.cs 94.59% <93.33%> (+2.00%) ⬆️
....LangServer/Completions/BicepCompletionProvider.cs 92.73% <93.61%> (+0.01%) ⬆️
...cep.LangServer.IntegrationTests/CompletionTests.cs 88.88% <95.74%> (+1.65%) ⬆️
...ntegrationTests/Helpers/TextDocumentParamHelper.cs 100.00% <100.00%> (ø)
....LangServer.IntegrationTests/SignatureHelpTests.cs 100.00% <100.00%> (ø)
... and 37 more

@majastrz majastrz force-pushed the majastrz/omnisharp-19 branch 2 times, most recently from c61a09a to b4737f1 Compare March 24, 2021 23:25
@majastrz majastrz force-pushed the majastrz/omnisharp-19 branch 4 times, most recently from 92aa8f2 to 74c2323 Compare April 1, 2021 04:15
@majastrz majastrz mentioned this pull request Apr 2, 2021
Closed
@majastrz
Copy link
Member Author

majastrz commented Apr 2, 2021

Opened OmniSharp/csharp-language-server-protocol#556 on the OmniSharp repo.

@majastrz majastrz force-pushed the majastrz/omnisharp-19 branch from fc8b0ca to 74ef2fa Compare April 23, 2021 02:47
@majastrz majastrz force-pushed the majastrz/omnisharp-19 branch 4 times, most recently from 54fdb79 to 31c8b46 Compare May 21, 2021 03:08
@majastrz majastrz force-pushed the majastrz/omnisharp-19 branch from 31c8b46 to 9b4eade Compare May 25, 2021 21:30
anthony-c-martin
anthony-c-martin approved these changes May 25, 2021
View reviewed changes
Copy link
Member

@anthony-c-martin anthony-c-martin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit::shipit::shipit::shipit:

@majastrz majastrz merged commit cf2a3ad into main May 25, 2021
@majastrz majastrz deleted the majastrz/omnisharp-19 branch May 25, 2021 23:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anthony-c-martin anthony-c-martin anthony-c-martin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.4
Development

Successfully merging this pull request may close these issues.
3 participants
@majastrz @codecov-io @anthony-c-martin