Skip to content

Conversation

david-driscoll
Copy link
Member

Two things:

  • Updated the router to pick the proper ITextDocumentSyncHandler previously this would pick the first one, which was only right if the one you wanted was first. No other handlers would.
  • ITextDocumentSyncHandler.GetTextDocumentAttributes can return null, previously this was assumed to be not null.
    • However one of your handlers should return a non null value or nothing will get routed.
  • Added some tests around LspRequestRouter to ensure it's working as intended (I hope, 😱)
  • Fixes Conversion of unsupported ClientCapabilities is incompatible with other LSP implementations. #35 using the hacky feeling ShouldSerializeX in Newtonsoft.Json. There is be a better way I'm sure, but these methods will be hidden from intellisense for consumers.

@david-driscoll
Copy link
Member Author

@tintoy going to push this as 0.4.0, let me know if you run into any problems.

@david-driscoll david-driscoll merged commit 99ce493 into master Oct 17, 2017
@david-driscoll david-driscoll deleted the fix/lsprouter-support-manyhandlers-tests branch October 17, 2017 19:12
@tintoy
Copy link
Collaborator

tintoy commented Oct 17, 2017

Cheers! Will give it a go in an hour or so

@tintoy
Copy link
Collaborator

tintoy commented Oct 17, 2017

What's the expected behaviour for requests / notifications where multiple handlers for the same method are registered?

@tintoy
Copy link
Collaborator

tintoy commented Oct 17, 2017

Ah, needs a logger factory now; will add one.

tintoy added a commit to tintoy/msbuild-project-tools-vscode that referenced this pull request Oct 17, 2017
Add Microsoft.Extensions.Logging and hook it up to Serilog.

OmniSharp/csharp-language-server-protocol#36
@tintoy
Copy link
Collaborator

tintoy commented Oct 17, 2017

Yep! Works fine, cheers :)

tintoy added a commit to tintoy/dotnet-language-client that referenced this pull request Oct 17, 2017
@tintoy
Copy link
Collaborator

tintoy commented Oct 17, 2017

Oops, spoke too soon:

System.NullReferenceException: Object reference not set to an instance of an object.
   at OmniSharp.Extensions.LanguageServer.LanguageServer.HasHandler[T](DynamicCapability capability)
   at OmniSharp.Extensions.LanguageServer.LanguageServer.<OmniSharp-Extensions-JsonRpc-IRequestHandler<OmniSharp-Extensions-LanguageServer-Models-InitializeParams\,OmniSharp-Extensions-LanguageServer-Models-InitializeResult>-Handle>d__26.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at OmniSharp.Extensions.LanguageServer.LspRequestRouter.<RouteRequest>d__9.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at OmniSharp.Extensions.JsonRpc.InputHandler.<>c__DisplayClass17_0.<<HandleRequest>b__3>d.MoveNext()

As shown here:

https://github.com/tintoy/dotnet-language-client/blob/master/samples/SingleProcess/Program.cs#L173

@tintoy
Copy link
Collaborator

tintoy commented Oct 17, 2017

Looks like the overload of HasHandler<T> that takes DynamicCapability (class) instead of SupportsDynamicCapability<T> (struct) needs to do a null-check; IIRC, it's legal for those fields to be null.

@david-driscoll
Copy link
Member Author

I didn't get to this last night... fixing shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants