[WIP] bring up to date with latest spec #51
Conversation
|
Fair point I'll add a wrapper interface. |
Codecov Report
@@ Coverage Diff @@
## master #51 +/- ##
==========================================
+ Coverage 70% 70.02% +0.02%
==========================================
Files 223 211 -12
Lines 2830 2202 -628
==========================================
- Hits 1981 1542 -439
+ Misses 849 660 -189
Continue to review full report at Codecov.
|
|
cc @tintoy if you get a chance to look it over... For serialization in the client I had to use a singleton instance of the new Serialzier, this needs to be fixed. Other than that all the serialization logic is now coupled to the Contract Resolver and Converter classes. |
|
I notice the empty notification handler delegate was removed - are notifications without payloads no longer supported? |
| @@ -50,7 +52,7 @@ public DelegateNotificationHandler(string method, NotificationHandler<TNotificat | |||
| await Task.Yield(); | |||
|
|
|||
| Handler( | |||
| notification != null ? notification.ToObject<TNotification>() : default(TNotification) | |||
| notification != null ? notification.ToObject<TNotification>(Serializer.Instance.JsonSerializer /* Fix me: this is ugly */) : default(TNotification) | |||
There was a problem hiding this comment.
Hmm, I'm thinking maybe we could make the the JsonSerializerSettings assignable via a property on ICustomizeJsonSerializer (new interface with a SerializerSettings property that INotificationHandler and friends could inherit from)? Either that or pass it into the Invoke() methods but the property seems cleaner.
There was a problem hiding this comment.
What if we change Invoke to take an object instead and deserialize the value before we call Invoke?
There was a problem hiding this comment.
Yeah that would be simpler - nice one :)
There was a problem hiding this comment.
Mind you, how do you know which type to deserialise into? I think that's why I did it in the handler because the parameter type is known.
There was a problem hiding this comment.
I suppose the handler could expose a ParametersType / PayloadType property on IInvokeNotificationHandler / IInvokeRequestHandler?
|
I think I'm going to land this for now, and we can come back to the serialization problem. The behavior hasn't changed, the code just isn't as nice. Let me know what you think. |
|
Sure works for me :) |
|
Want me to open a PR for fixing up the client serialiser stuff? |
This is so the common JsonSerializerSettings can be used by the calling code. NOTE: This code is currently non-functional - handlers have been modified to take an Object instead of a JObject but the calling code does not yet perform deserialisation.
…ation Move payload deserialisation out of handlers
|
Getting back to things after the holidays, everything is passing, going to merge, let me know if you see any problems! |
Things left to do...
IXOptionsinterfaces with capabilitiesThings to figure out
MarkedStringwas deprecated but it should still be supported for LSP 2.0 and existing LSP clients. We havecontentFormatthat we can run off of.JsonSerializerorJsonSerializerSettingsmanually. Today we cheat and just use the static methods. With this latest change we have to adapt our serialization to what the editor supports.