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

Migrate to python-lsp-server ? #18

Merged
merged 1 commit into from
Jan 26, 2022

Conversation

guillaumealgis
Copy link
Contributor

@guillaumealgis guillaumealgis commented Dec 4, 2021

The original Palantir python-language-server project seems to be abandoned, and several LSP plugins are moving to its community fork python-lsp-server.

How do you feel about moving the Nova plugin to python-lsp-server too?


If you think this is a good idea, I also could work on prompting the user to install the python-lsp-server package if we detect that python-language-server is installed when launching this plugin (in another PR).

(Also, my branch name is a bit off because I initially though your were already recommending using the fork in the README, and so I thought we only had to update the settings key).

@guillaumealgis guillaumealgis changed the title Use python-lsp-server settings namespace Migrate to python-lsp-server ? Dec 4, 2021
@mmshivesh
Copy link
Owner

You are not wrong. I actually talked about that fork a bit here. That language server seems to be a drop in replacement for the Palantir's LS, so that would be a good option to look into especially since we cannot use PyLance.

It should be a simple replacement to switch over to this Language Server, and I think it's a good idea to do so especially if the fork is actively maintained.

If you think this is a good idea, I also could work on prompting the user to install the python-lsp-server package if we detect that python-language-server is installed when launching this plugin (in another PR).

A great QoL idea. I haven't been working as much as I want on this extension especially because I don't have enough JS experience to do justice to this extension. If you can add additional features, go right ahead and open PRs, i'll add them in.

@guillaumealgis
Copy link
Contributor Author

@mmshivesh I updated the PR to be a bit more "smart" and open about the switch, by using the backend available to us on the user's machine (with a small bias towards pylsp which is checked first).

  • The settings "namespace" is determined by the name of the backend we are using, so both are supported
  • The pyls.executable preference is honored, but if it's not defined explicitly we're checking if we can find a suitable executable in the user's $PATH (checking for pylsp first, then pyls)

A great QoL idea. I haven't been working as much as I want on this extension especially because I don't have enough JS experience to do justice to this extension. If you can add additional features, go right ahead and open PRs, i'll add them in.

Great! I'll work on than when I find the time, then :)

@mmshivesh
Copy link
Owner

This looks good so far, I'll go ahead and merge this if it's okay?

@guillaumealgis
Copy link
Contributor Author

@mmshivesh ok for me, I've been using my forked version of the plugin for a while now and had no issue

@mmshivesh mmshivesh merged commit 731721d into mmshivesh:master Jan 26, 2022
@guillaumealgis guillaumealgis deleted the pylsp-settings-namespace branch March 7, 2022 20:30
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.

None yet

2 participants