search

lite-xl / lite-xl-lsp-servers

Public repo of easily usable lsp servers, that interface with the lite-xl plugin system.
MIT License
8 stars 10 forks source link

Add python lsp support via Pyright #14

Closed Gaspartcho closed 3 months ago

Gaspartcho commented 4 months ago

The version of Pyright used here is the version packaged by Node-Js (hence the download link in manifest.json heading to their domain).

I am not sure about the checksum value, as this is the first time I am working with this. (I generated it form https://emn178.github.io/online-tools/sha256_checksum.html)

This PR should resolve #8 , even though it is not using the same server to do it.

Guldoman commented 4 months ago

It looks like this won't work if the user doesn't have node installed on their system, right? If that's the case we can't merge this, as the plugins in this repo are supposed to automatically make the servers work.

You'd need to also create a node library as a dependency of lsp_python, in a similar way that's being done in #12.

Gaspartcho commented 4 months ago

Yes, I haven't noticed, I'll get right to it.

Also I noticed that NodeJs provides more binaries than the four already specified ones ("x86_64-linux", "x86_64-windows", "x86_64-darwin", "aarch64-darwin"). Should I add the remaining?

Guldoman commented 4 months ago

Also I noticed that NodeJs provides more binaries than the four already specified ones ("x86_64-linux", "x86_64-windows", "x86_64-darwin", "aarch64-darwin"). Should I add the remaining?

Sure, feel free to.

PerilousBooklet commented 4 months ago

The nodejs dependency should have the optional property set to true, to allow people who already have it installed to choose not to install it.

PerilousBooklet commented 4 months ago

Did you test it?

Gaspartcho commented 4 months ago

Just did. Seems to work fine.

EDIT: Except for installing, because lpm still crashes each time I try to install the nodejs library

EDIT 2: Also I have forgotten to update the installed path for the library in the lua script, should be updated now

Gaspartcho commented 4 months ago

I just realized that the file extension for the Pyright archive was in .tgz, which would prevent it from being automatically extracted by lpm on install. However, from what I understood, the only difference with a .tar.gzfile would be the file name itself.

What should I do? Try to add .tgz support to lpm (by modifying one line), or change mirror for Pyright?

note: I initially choose to take the package provided by npm because it was lighter than the GitHub version and did not contained the Vs Code extension.

PerilousBooklet commented 4 months ago

The lighter the better.

PerilousBooklet commented 4 months ago

Things to do:

Guldoman commented 4 months ago

This last change was not correct. What issues did you get with language_python as a dependency?

Gaspartcho commented 4 months ago

I was running some tests, and I tried to uninstall lsp_python, and lpm tried to uninstall language_python, as it became an orphan dependency, but failed, as it is a core addon.

Maybe there was a better solution though...

Guldoman commented 4 months ago

If that's the case, this is an LPM bug. Feel free to report it, or I'll do it later.

Gaspartcho commented 4 months ago

Turns out that what was preventing the nodejs library to be installed was that the archive containing the binaries used the GNU TAR format, which was different from the one supported by microtar (v7). I've submitted a PR which should fix this (I tested it and it looks like it was working).

PerilousBooklet commented 4 months ago

Is it ready to be submitted for merge?

Gaspartcho commented 4 months ago

Is there anything else I need to add or is this PR ready to be merged?

Guldoman commented 4 months ago

I think the only decision at this point is about the optionality of nodejs, @adamharrison.

adamharrison commented 3 months ago

Tested this, looks good to me!