-
Notifications
You must be signed in to change notification settings - Fork 268
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.
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
Update the struct definitions to match version 3.14 of the LSP spec #202
Conversation
@swift-ci Please test |
@swift-ci Please test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few high-level comments inline. I haven't reviewed the details yet.
In cases where alternative result types are subsumed by each other, is there a reason to preserve the exact structure? For example, a CompletionList
contains all the information from [CompletionItem]
, so is there a good reason to have the indirection of a new response type instead of just teaching CompletionList
to decode from an array in addition to a dictionary? The only thing we lose is the ability to round-trip the serialized form, which doesn't seem very important. Similarly, Location
vs [Location]
with only one element.
This doesn't apply in every case, but where possible it seems nice to keep the programmatic API simpler.
@@ -45,6 +45,16 @@ public struct PositionRange: CustomCodableWrapper { | |||
} | |||
} | |||
|
|||
extension Optional: CustomCodableWrapper where Wrapped == PositionRange { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't quite sufficient, because it will encode as "range": null
. LSP explicitly says we need to omit the key instead (except when the spec explicitly says | null
in the allowed values.
I recently learned how to make this work, so once #204 is in you should be able to rebase 😄
// | ||
//===----------------------------------------------------------------------===// | ||
|
||
extension Character: Codable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't tie Swift's definition of a Character to serialization - for one thing, the definition of a Character changes over time because of changes to unicode. It's also unlikely to ever match what other lsp implementations use to define a character. The spec itself uses String.
I did the following change to address the review feedback:
I also split a couple commits off this PR to keep it at a more manageable size. They now live in #206. |
@swift-ci Please test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically LGTM. One nitpick
// Fallback: Decode single location as array with one element | ||
self = .locations([location]) | ||
} else { | ||
let context = DecodingError.Context(codingPath: decoder.codingPath, debugDescription: "DefinitionResponse could neither be decoded as [Location], nor as [LocationLink], nor as Location") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"DefinitionResponse" may be incorrect; I think you can just drop it. Maybe something like
"expected [Location], [LocationLink], or Location"
@swift-ci Please test |
I went through the spec, comparing the struct definitions to what is defined in https://microsoft.github.io/language-server-protocol/specifications/specification-3-14/ and changed the definitions accordingly to match the spec.