FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Add web sockets support by npradeep357 · Pull Request #128 · python-lsp/python-lsp-server · GitHub

/ python-lsp-server Public

Add web sockets support - #128

Merged
ccordoba12 merged 30 commits into
python-lsp:developfrom
npradeep357:develop
May 30, 2022
ConversationCommits (30)ChecksFiles changed
Merged

Add web sockets support#128
ccordoba12 merged 30 commits into
python-lsp:developfrom
npradeep357:develop

Conversation

npradeep357 commented Nov 30, 2021 •
edited by ccordoba12
Loading

Copy link
Copy Markdown
Contributor

This pull request will enable PYLSP to run in Web Sockets configuration and can handle multiple connections in parallel.
The Idea was to handle multiple parallel editing sessions using single process. This is achieved by creating new instances of PYLSP class Object for each WS connection in a single server process.

Fixes #117

As of now this PR only supports text based messaging over WS. Binary messaging support will be added in future.

This PR uses autobahn websockets with twisted framework Websockets Library
There is asyncio based framework but twisted is used because PYLSP is based on callbacks and asyncio is not compatible with it.

referred docs:
https://autobahn.readthedocs.io/en/latest/websocket/programming.html
https://autobahn.readthedocs.io/en/latest/installation.html
https://websockets.readthedocs.io/en/stable/

related dependencies are added to setup.py setup.cfg
related usage documentation is added to README

Pradeep Neerukonda and others added 3 commits December 1, 2021 00:20
npradeep357 mentioned this pull request Nov 30, 2021
Closed
ccordoba12 requested a review from krassowski December 1, 2021 01:58
ccordoba12 reviewed Dec 1, 2021
View reviewed changes

ccordoba12 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Hey @npradeep357, thanks a lot for your contribution! My main comment for now is it's possible to add some tests for this new functionality, given that right now I see none.

Besides that, I'd like to ask @krassowski what he thinks about this one.

npradeep357 commented Dec 1, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@ccordoba12, Thank you, will add tests for this.

Also, regarding the checks, there are Linux, Mac and Windows tests failure which says the version mentioned is not available for Py 3.6. I took the latest version of autobahn from PyPI which is 21.11.1.

But the log in the tests shows only until 21.2.1 which is an older version released in Feb 2021. ref: autobahn PyPI history

Suggest me if I have to move to older version or is there something I'm missing for Py 3.6?

npradeep357 commented Dec 1, 2021

Copy link
Copy Markdown
Contributor Author

After some digging, I found out that autobahn is only supported from Python 3.7+.

Is there a reason that PYLSP is supported from Python 3.6+? Can we move PYLSP to Python 3.7+? or should I downgrade the autobahn library which I used?

ccordoba12 commented Dec 1, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

Is there a reason that PYLSP is supported from Python 3.6+?

There is because Python 3.6 is still a supported Python version. However, since it's going to reach end-of-life this month, we can drop support for it now.

Please do that in a different pull request though, i.e. increase the Python version in our setup.py and remove the Python 3.6 slots on Github actions. After that you could rebase this PR so that your tests pass.

npradeep357 mentioned this pull request Dec 1, 2021
Merged

npradeep357 commented Dec 3, 2021

Copy link
Copy Markdown
Contributor Author

Hey @ccordoba12, I have created a separate PR for version change as mentioned. Can you review and approve #130 when you find some time. Thank you

npradeep357 mentioned this pull request Dec 3, 2021
Closed
krassowski reviewed Dec 3, 2021
View reviewed changes
Comment thread README.md Outdated
krassowski reviewed Dec 3, 2021
View reviewed changes
Comment thread README.md Outdated
krassowski reviewed Dec 3, 2021
View reviewed changes
Comment thread README.md Outdated
krassowski reviewed Dec 3, 2021
View reviewed changes
Comment thread pylsp/python_lsp.py Outdated
krassowski reviewed Dec 3, 2021
View reviewed changes
Comment thread setup.py Outdated
- moved websockets information to end of configuration
25e3fab 
- renamed extras - autobahn-twisted to websockets
- added more info on the import error

npradeep357 commented Dec 4, 2021

Copy link
Copy Markdown
Contributor Author

@krassowski made the suggested changes.

krassowski reviewed Dec 4, 2021
View reviewed changes
Comment thread pylsp/python_lsp.py Outdated
krassowski reviewed Dec 4, 2021
View reviewed changes
Comment thread pylsp/python_lsp.py Outdated

krassowski commented Dec 4, 2021

Copy link
Copy Markdown
Member

Thanks! Looks fine by me, only minor comments now.

krassowski approved these changes Dec 6, 2021
View reviewed changes

krassowski left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Broadly speaking it looks ok for me. Not sure about handling of the binary messages - should it emit a warning when one is received?

ccordoba12 added this to the v1.5.0 milestone Apr 4, 2022
ccordoba12 reviewed Apr 4, 2022
View reviewed changes

ccordoba12 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@npradeep357, thanks for your patience with this! Last review and then this should be ready (we can take a look at adding tests later).

Comment thread README.md Outdated
Comment thread README.md Outdated
Comment thread pylsp/python_lsp.py Outdated
Comment thread pylsp/python_lsp.py Outdated
Comment thread pylsp/python_lsp.py Outdated

npradeep357 commented Apr 4, 2022

Copy link
Copy Markdown
Contributor Author

@npradeep357, thanks for your patience with this! Last review and then this should be ready (we can take a look at adding tests later).

@ccordoba12 I'm planning to change the websockets library to reduce 3rd party library footprint. If it can wait a week or two, I will update the PR.

ccordoba12 commented Apr 4, 2022

Copy link
Copy Markdown
Member

Sure, no problem with that.

npradeep357 and others added 5 commits April 14, 2022 11:06
Merge branch 'python-lsp:develop' into develop
3bae1c5
Websockets (#4)
a977e33 
* Added websockets based implementation to reduce libraries and 3rd party dependencies

* Removed message encode and decode as they are no longer needed

Co-authored-by: Pradeep Neerukonda <pneeruko@opentext.com>
Update README.md
1de7296
removed triling whitespaces
330bab6
removed space between parameters
0cb47b5

npradeep357 commented Apr 21, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@ccordoba12, @krassowski, removed autobahn and twisted libraries and using websockets library to reduce libraries footprint.
please review, approve and merge this.

I have tested it my application and everything's working as expected..

websockets: https://websockets.readthedocs.io/en/stable/

npradeep357 commented May 3, 2022

Copy link
Copy Markdown
Contributor Author

@ccordoba12 @krassowski could you please review the latest changes.

npradeep357 commented May 17, 2022

Copy link
Copy Markdown
Contributor Author

@ccordoba12, can we merge this?

ccordoba12 reviewed May 28, 2022
View reviewed changes

ccordoba12 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@npradeep357, thanks for your patience with this! There are still two unaddressed suggestions from my previous review. After you apply them, I'll merge your PR.

Comment thread README.md Outdated
Comment thread pylsp/python_lsp.py Outdated
ccordoba12 changed the title Web Sockets support Add web Sockets support May 28, 2022
ccordoba12 changed the title Add web Sockets support Add web sockets support May 28, 2022

npradeep357 commented May 30, 2022

Copy link
Copy Markdown
Contributor Author

Hi @ccordoba12, Somehow these suggestions got overlooked. I have addressed them now. Thank you

ccordoba12 approved these changes May 30, 2022
View reviewed changes

ccordoba12 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks for your contribution @npradeep357!

ccordoba12 merged commit 62b7cc6 into python-lsp:develop May 30, 2022
This file contains hidden or 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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

ccordoba12 ccordoba12 approved these changes
krassowski Awaiting requested review from krassowski

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v1.5.0

Development

Successfully merging this pull request may close these issues.

Websockets built-in support

3 participants

Back | FazBrowse Home | New Git URL