Skip field when it is null/none. #41
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There seems to be some discrepancy around field being ommited and null value. And null fields are breaking at least some fragile language servers. - autozimu/LanguageClient-neovim#265 - jacobdufault/cquery#247
|
I thought I had read it to be equivalent in microsoft/language-server-protocol#87 but re-reading that now I seem to have misread it. We should perhaps mark every optional field in the spec with I queried for more information in a related issue microsoft/language-server-protocol#355 |
|
In #87 they deferred the work to specific language implementation but as
far as I see it’s simply a total failure to commicate clearly how it’s
going to work in a real world of multiple language implementations.
I agree we should do all the skip serialization work, just to be safe.
…On Sun, Jan 7, 2018 at 02:33 Markus Westerlind ***@***.***> wrote:
I thought I had read it to be equivalent in
microsoft/language-server-protocol#87
<microsoft/language-server-protocol#87> but
re-reading that now I seem to have misread it. We should perhaps mark every
optional field in the spec with skip_serializing_if actually, just to be
safe 🤔 .
I queried for more information in a related issue
microsoft/language-server-protocol#355
<microsoft/language-server-protocol#355>
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#41 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABYt77ZZQ5TUIbklP2R5I-devhsqtVKOks5tIJ2SgaJpZM4RViXn>
.
|
|
Can we merge this in at the moment? The rest could be done in separate PR. Also, it would be really helpful to have the attribute applied globally instead of individual field. |
|
Published 0.24.0. @autozimu Would you like want merge rights/cargo publish rights? I think it might be nice to have someone else with access (also, I will be mostly unavailable this week in case you want something merged/published). |
|
That will be great. Thanks.
My handle on crates.io is same as github.
…On Mon, Jan 8, 2018 at 18:39 Markus Westerlind ***@***.***> wrote:
Published 0.24.0. @autozimu <https://github.com/autozimu> Would you like
want merge rights/cargo publish rights? I think it might be nice to have
someone else with access (also, I will be mostly unavailable this week in
case you want something merged/published).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#41 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABYt7xXJYDxoEbeqc8k9K2k4LT16MdYFks5tItFMgaJpZM4RViXn>
.
|
|
Done |
|
Thanks |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There seems to be some discrepancy around field being ommited and null
value. And null fields are breaking at least some fragile language
servers.