Initial port of LSP client from tintoy/dotnet-language-client. #43

tintoy · 2017-11-03T21:18:45Z

Hey - as discussed in #42, I've pulled everything into the OmniSharp.LanguageServerProtocol.Client namespace. Have tried to change as little as possible as part of the port so we can talk it over in its present state.

BTW, the client library has to target netstandard2.0 (netstandard1.6 doesn't have the required support for Pipes).

tintoy · 2017-11-04T02:28:44Z

It looks like CI may be failing due to an unrelated issue?

Error finding repository for 'https://ci.appveyor.com/nuget/csharp-language-server-protocol': An error occurred while retrieving package metadata for 'xunit.runner.console.2.3.0' from source 'AppVeyorProjectFeed'.

tintoy · 2017-11-04T02:32:09Z

Ah, no, I see now. Need the dotnet-xunit CLI tool.

…build process).

…project.

codecov · 2017-11-04T02:39:51Z

Codecov Report

Merging #43 into master will decrease coverage by 11.16%.
The diff coverage is 34.49%.

@@             Coverage Diff             @@
##           master      #43       +/-   ##
===========================================
- Coverage   66.38%   55.21%   -11.17%     
===========================================
  Files         193      233       +40     
  Lines        2041     3142     +1101     
===========================================
+ Hits         1355     1735      +380     
- Misses        686     1407      +721

Impacted Files	Coverage Δ
src/Client/Logging/LoggerExtensions.cs	`0% <0%> (ø)`
src/Client/Clients/WorkspaceClient.cs	`0% <0%> (ø)`
src/Client/LanguageClient.cs	`0% <0%> (ø)`
src/Client/Clients/TextDocumentClient.Hover.cs	`0% <0%> (ø)`
src/Client/Handlers/DelegateNotificationHandler.cs	`0% <0%> (ø)`
src/Client/Protocol/ErrorMessage.cs	`0% <0%> (ø)`
src/Client/Clients/TextDocumentClient.Sync.cs	`0% <0%> (ø)`
src/Client/Protocol/LspConnectionExtensions.cs	`0% <0%> (ø)`
...Client/Handlers/JsonRpcEmptyNotificationHandler.cs	`0% <0%> (ø)`
src/Client/Exceptions.cs	`0% <0%> (ø)`
... and 73 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 80e1b38...c87fd85. Read the comment docs.

david-driscoll · 2017-11-08T16:27:34Z

looking at this today, sorry I completely forgot about it this weekend.

david-driscoll

Looks good to me... there are somethings I'd like to change, but I'm not going to block merging this for them. The timezone difference itself is enough, lol.

If can make some of the changes, great, if you need to do them later I say merge it in and we'll make some breaking changes and bump the version later.

david-driscoll · 2017-11-08T16:32:54Z

src/Client/Client.csproj

+
+    <ItemGroup>
+        <PackageReference Include="Newtonsoft.Json" Version="10.0.3" />
+        <PackageReference Include="Serilog" Version="2.5.0" />


I'm hesitant to depend on Serilog by itself here, as I would generally prefer to not prescribe logging frameworks for consumers.

However Serilog is a great logger, so I'm okay with it for now, and perhaps refactor it away (for MS.Extensions) in a future (breaking) change.

Thanks for the reminder - I'm fine with changing over to MEL for logging, I just forgot about it :)

I'll modify them to take an ILogger / ILoggerFactory instead (some components need to pass a different ILogger<TComponent> to child components).

david-driscoll · 2017-11-08T16:34:14Z

src/Client/Clients/TextDocumentClient.Completions.cs

+
+namespace OmniSharp.Extensions.LanguageServerProtocol.Client.Clients
+{
+    using Utilities;


nit: Not a huge fan of nested usings.

No problem; I tend to use it as a reminder of which namespaces are from "my" code vs external code, but I'm happy to change this over. My rule is usually "match the style already used by the project" :)

david-driscoll · 2017-11-08T16:35:42Z

src/Client/Clients/TextDocumentClient.Completions.cs

+            if (String.IsNullOrWhiteSpace(filePath))
+                throw new ArgumentException($"Argument cannot be null, empty, or entirely composed of whitespace: {nameof(filePath)}.", nameof(filePath));
+
+            return PositionalRequest<CompletionList>("textDocument/completion", filePath, line, column, cancellationToken);


future: Now that the strings are showing up in more places we should probably add a static constants class somewhere. I'll add an issue for this.

Yeah, I probably should have done this in the first place (even when my project was stand-alone) but haven't got around to it.

david-driscoll · 2017-11-08T16:37:24Z

src/Client/Clients/TextDocumentClient.Completions.cs

+        /// <returns>
+        ///     A <see cref="Task{TResult}"/> that resolves to the completions or <c>null</c> if no completions are available at the specified position.
+        /// </returns>
+        public Task<CompletionList> Completions(Uri documentUri, int line, int column, CancellationToken cancellationToken = default(CancellationToken))


We could probably make this a wrapper for the other method, and just pass documentUri as a string or better yet reverse it so that we pass filePath into this method. That would ensure that the string is a proper Uri.

Yep, good catch :)

david-driscoll · 2017-11-08T16:44:49Z

src/Client/HandlerDelegates.cs

+    /// <returns>
+    ///     A <see cref="Task"/> representing the operation.
+    /// </returns>
+    public delegate void EmptyNotificationHandler();


Should be safe to rename to NotificationHandler.

Yeah, I was trying to make it explicit to consumers that empty notifications are not the same as notifications with payloads (the language server implementation will freak out if you don't pass a payload when one is expected.

But I agree it makes more sense to just vary by type parameters.

I've changed this, but I'd like to keep the underlying TryHandleEmptyNotification methods where there are no generic type parameters because I got caught more than once by accidentally calling the wrong overload.

david-driscoll · 2017-11-08T16:47:34Z

src/Client/HandlerDelegates.cs

+    /// <param name="notification">
+    ///     The notification message.
+    /// </param>
+    public delegate void NotificationHandler<TNotification>(TNotification notification);


I recently changed these on the server to be Task instead of void to allow them to block processing until the operation completes, this ensures things like text updates are completed.

Question: Should we do the same here for the client? (I honestly don't know)

Interesting point; I think that makes sense server-side, but I reckon sending a notification should be fire-and-forget. I can't think of a reason that a client would want to block until it's been sent.

and thats why I ask the question. :D

david-driscoll · 2017-11-08T16:50:10Z

src/Client/Handlers/IHandler.cs

+        /// <summary>
+        ///     The kind of handler.
+        /// </summary>
+        HandlerKind Kind { get; }


HandlerKind and the Kind property seem unnecessary. Worst case we can pattern match, but currently they aren't used anywhere but setting them..

My original thinking on this was that the handlers themselves would be generic and so it would be hard to pattern-match on them without already knowing the handler payload type (but you're right it's probably redundant now).

david-driscoll · 2017-11-08T17:01:28Z

src/Client/Protocol/LspConnection.cs

+            }
+
+#pragma warning disable CS4014 // Continuation does the work we need; no need to await it as this would tie up the dispatch loop.
+            handlerTask.ContinueWith(_ =>


This will throw if handlerTask is null, should probably return in the above if.

Hah! Yes, good catch :)

tintoy · 2017-11-08T18:51:20Z

Hi - I'll sort all these out over the next couple of days, thanks for taking the time to do a detailed review :)

…that take document URIs (OmniSharp#43).

OmniSharp#43).

tintoy · 2017-11-08T19:32:39Z

Ok - the only thing left to do (I think) is the switch from Serilog to MEL. That may take a little longer; Just need to make sure I'm not directly using Serilog-specific functionality anywhere and I'll need to rework the test framework to handle this (but that should be doable).

tintoy · 2017-11-08T19:33:43Z

I'll also see what I can do to bump up the test coverage.

tintoy · 2017-11-08T20:57:57Z

Logging switched over, too - that turned out to be easier than I expected 😁

david-driscoll · 2017-11-08T21:01:39Z

FYI For logging I'm totally fine you bring in Serilog and MEL to handle logging into Xunit, it just makes things so much easier.

I would just prefer that the external interface tie to MEL if possible.

tintoy · 2017-11-09T00:47:31Z

No worries - turns out it was easy to use MEL for in-test logging (see the TestBase class for details).

tintoy · 2017-11-09T00:48:20Z

We could probably also create an ILoggerProvider to log to the language server via window/logMessage :)

tintoy · 2017-11-09T00:48:56Z

But if you want the Serilog logging stuff for Xunit, I'm happy to contribute that too (although it's no longer needed here).

david-driscoll · 2017-11-09T00:49:51Z

If it works we'll leave it alone for now. 👍

tintoy · 2017-11-09T00:54:11Z

😉

david-driscoll · 2017-11-15T15:49:46Z

Hey Adam, were you going to try and bring up the coverage anymore? If not I'm fine merging this as is, and we can add additional coverage later.

tintoy · 2017-11-15T16:00:17Z

Yeah sorry that was on my list for this morning :)

tintoy · 2017-11-15T16:37:29Z

Actually given all the other stuff going on this morning, perhaps you can merge it and I'll open a new PR for the additional tests sometime tonight (at conference today)?

tintoy · 2017-11-15T18:45:19Z

Woohoo!

tintoy added 3 commits November 4, 2017 08:15

Initial port of LSP client from tintoy/dotnet-language-client.

db0ee47

Fix up assembly names and root namespaces for LSP client and its tests.

b8defce

Remove public modifier from _dispatcher field in LspConnection (typo).

5f4a15b

tintoy requested a review from david-driscoll November 4, 2017 02:29

tintoy added 2 commits November 4, 2017 13:34

Add dotnet-xunit CLI package to Client.Tests project (required by CI …

8b7de8c

…build process).

Use stable version of Microsoft.NET.Test.Sdk package in Client.Tests …

e400183

…project.

david-driscoll approved these changes Nov 8, 2017

View reviewed changes

tintoy added 5 commits November 9, 2017 06:07

Bail out if handler returns a null task (OmniSharp#43).

1496e90

Replace nested namespace imports with top-level ones (OmniSharp#43).

50f4e87

Client methods that take file-system paths now delegate to overloads …

cd0c4e6

…that take document URIs (OmniSharp#43).

Rename EmptyNotificationHandler to NotificationHandler where practical (

f16396f

OmniSharp#43).

Remove unused HandlerKind enumeration (OmniSharp#43).

52070bf

Switch from Serilog to Microsoft.Extensions.Logging (OmniSharp#43).

c87fd85

david-driscoll merged commit fe0a81a into OmniSharp:master Nov 15, 2017

Initial port of LSP client from tintoy/dotnet-language-client. #43

Initial port of LSP client from tintoy/dotnet-language-client. #43

Conversation

tintoy commented Nov 3, 2017 • edited Loading

tintoy commented Nov 4, 2017

tintoy commented Nov 4, 2017

codecov bot commented Nov 4, 2017 • edited Loading

Codecov Report

david-driscoll commented Nov 8, 2017

david-driscoll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tintoy commented Nov 8, 2017

tintoy commented Nov 8, 2017

tintoy commented Nov 8, 2017

tintoy commented Nov 8, 2017

david-driscoll commented Nov 8, 2017

tintoy commented Nov 9, 2017

tintoy commented Nov 9, 2017

tintoy commented Nov 9, 2017

david-driscoll commented Nov 9, 2017

tintoy commented Nov 9, 2017

david-driscoll commented Nov 15, 2017

tintoy commented Nov 15, 2017

tintoy commented Nov 15, 2017

tintoy commented Nov 15, 2017

tintoy commented Nov 3, 2017 •

edited

Loading

codecov bot commented Nov 4, 2017 •

edited

Loading