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 WebSocket server #129

Merged
merged 8 commits into from
Jun 16, 2021
Merged

Conversation

MatejKastak
Copy link
Contributor

Description

Implementation of WebSocket server. This PR allows users to start the WebSocket server on a given host and port that will "wrap" the LSP protocol. The interface is the same as other SERVER.start_* methods.

The main use case is to create convenient interface to interact with editors that are embedded in internet browsers (for example Monaco LSP-client), this could potentially allow for broader adoption.

Related to #85

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)

@MatejKastak MatejKastak marked this pull request as ready for review July 18, 2020 22:17
@MatejKastak
Copy link
Contributor Author

I think PR is at the point when it's ready for review/discussion.

  1. Adding websockets as a dependency made some of the tests fail, how would you like to resolve this problem?

    • websockets lib should work on >= python 3.6 based on their docs (We can probably make 3.6 pass)
    • what about python 3.5? You probably want to keed support for 3.5
    • we can make WebSockets feature not default but rather opt-in, and let users install it via pip install pygls[websockets] for example. After make some import guard to start_websocket and throw runtime error if the package is not found. The tests for this variant would run only on >= 3.6.
  2. How to handle pathargument - code

    • we can add optional or madnatory argument to start_websocket for user to specify exact path on which the server should listen
    • we can discard it and listen on all endpoints

Any feedback appreachiated.

Copy link
Contributor

@danixeee danixeee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MatejKastak Sorry for the delayed response. We have removed support for python 3.5, so that's not a problem anymore. Take a look at the inline comments and let me know what do you think about how we should solve them.

pygls/protocol.py Outdated Show resolved Hide resolved
pygls/server.py Outdated Show resolved Hide resolved
@danixeee
Copy link
Contributor

@MatejKastak I just pushed minor updates, thanks and sorry for waiting this long.

Copy link
Contributor

@danixeee danixeee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works fine with Monaco client.

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