Skip to content
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

Add library-specific strategy to address javascript's Optional Properties #132

Closed
wants to merge 2 commits into from

Conversation

pappasam
Copy link
Contributor

@pappasam pappasam commented Aug 20, 2020

Description

Resolves #124 by implementing this suggestion: #124 (comment)

Basically, this creates a Python-equivalent of typescript undefined for this library. None == null, undefined == undefined, allowing our serialized messages to more-closely match up with the LSP.

I've tested this E2E with https://github.com/pappasam/jedi-language-server and it works in practice for that language server.

A couple unit tests are now broken (the tests relied on the "bug" identified in https://github.com/pappasam/jedi-language-server). Unfortunately I don't have enough time to revise the tests, but if you like this implementation, I'd merge and have someone more familiar with the tests revise them to more-closely resemble the LSP's spec.

Code review checklist (for code reviewer to complete)

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
  • Title summarizes what is changing
  • Commit messages are meaningful (see this for details)
  • Tests have been included and/or updated, as appropriate
  • Docstrings have been included and/or updated, as appropriate
  • Standalone docs have been updated accordingly
  • CONTRIBUTORS.md was updated, as appropriate
  • Changelog has been updated, as needed (see CHANGELOG.md)

@puremourning
Copy link

I just merged this locally after seeing the same bug in cmake-language-server

It solves it. Thanks.

Any likelihood of this getting attention from the maintainers ?

@danixeee danixeee self-requested a review January 15, 2021 11:28
@danixeee
Copy link
Contributor

@pappasam @puremourning Sorry, I missed to let you know about my PR that adds type checking and better (de)serialization.

I am waiting to get some feedback on it before I merge it. Please check this comment.
If you need any help please let me know.

This issue should be resolved there and optional properties won't be sent to the client. Do you still think that we should add logic for undefined? What if the language client is not written in javascript and does not have undefined?

@puremourning
Copy link

@danixeee the main problem as it is now is that pygls is sending "key": null for optional fields. This is invalid and the keys should not be supplied unless null is specified in LSP as a valid value for key. That's irrespective of the client language (my client is python, and expects the key to be supplied or not as in message.get( "key", default_value ).

If the other PR is going to resolve that anyway, then great.

@danixeee
Copy link
Contributor

@puremourning Ok, I will check it again.

We are now using pydantic for (de)serialization and there's an argument exclude_unset which removes keys which values are not provided, and keeps keys even if the value is set to None.

Would you have time and the will to test it on your language server?

@danixeee
Copy link
Contributor

@puremourning I just pushed changes that should address this issue.

@pappasam
Copy link
Contributor Author

@danixeee amazing, I'm thrilled this is going to be resolved on the next release! Any idea when that will be? I can't wait to use it when it stabilizes.

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.

Don't send not specified properties
3 participants