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

Prefer interpreter's Ruff to environment's Ruff #83

Merged
merged 1 commit into from
Mar 10, 2023
Merged

Conversation

charliermarsh
Copy link
Member

@charliermarsh charliermarsh merged commit 4524157 into main Mar 10, 2023
@charliermarsh charliermarsh deleted the charlie/env branch March 10, 2023 16:42
@yaegassy
Copy link
Contributor

yaegassy commented Mar 12, 2023

Hi, @charliermarsh

Starting with ruff-lsp v0.2.1, ruff installed in a virtual environment is no longer available. The virtual environment is already activated.

I have a question, Is it correct that importStrategy should be set to fromEnvironment if I want to use ruff in the virtual environment?

I am using Vim/Neovim, and the importStrategy setting may be working in VSCode, but maybe the importStrategy setting is not working as intended in non-VSCode environments.

@yaegassy
Copy link
Contributor

Hi, @rchl

Does the fromEnvironment setting in importStrategy work correctly in sublimelsp/LSP-ruff? If you can confirm this, I would appreciate it if you could let me know. https://github.com/charliermarsh/ruff-vscode#settings

@charliermarsh
Copy link
Member Author

Oh weird, I mean this definitely changed, but I wouldn't have expected it to break anything, since in both cases, we're only returning a valid path to Ruff (we just now prefer the activate interpreter's Ruff over a global Ruff). What behavior are you seeing?

@yaegassy
Copy link
Contributor

The current behavior is that the ruff command installed with the ruff-lsp dependency is used.

@yaegassy
Copy link
Contributor

I debugged it in code and it seems to match this conditional branch.

https://github.com/charliermarsh/ruff-lsp/blob/4da5c210df3fed7da9c9ccfdabdbf6e5bd636162/ruff_lsp/server.py#L810-L812

@charliermarsh
Copy link
Member Author

Mmm, yeah, I can see why that wouldn't work when ruff-lsp is installed in some other virtual environment, and why it would work in VS Code. What we had before was a bug though. We need different logic for the different cases... The thing is, VS Code does some work to ensure that the LSP runs within the current user's virtual environment (and bundles Ruff separately, so Ruff and the LSP aren't installed in that virtual environment, but they are run within it). I'm not sure if other clients have similar capabilities -- we kind of get this "for free" from VS Code, and the use of the LSP outside of VS Code thus has some vestiges of that initial implementation.

We might want to have separate logic for VS Code vs. other LSPs. Or we might want to have three settings here: fromBundled, fromEnvironment, and fromGlobal or something to indicate the which usage.

@yaegassy
Copy link
Contributor

Thanks for the explanation about VS Code and importStrategy.

In the meantime, the problem I have will be addressed with a patch on the language client and related plugins side.

@rchl
Copy link
Contributor

rchl commented Mar 12, 2023

Does the fromEnvironment setting in importStrategy work correctly in sublimelsp/LSP-ruff? If you can confirm this, I would appreciate it if you could let me know. charliermarsh/ruff-vscode#settings

We haven't yet updated to the latest ruff-lsp in LSP-ruff but when I try it locally it seems to work fine. In our case we always load ruff-lsp/ruff from the virtual environment created specifically for the the LSP-ruff package. So in that case it seems to work the same with both the old and the new version of rust-lsp.

@yaegassy
Copy link
Contributor

@rchl Thank you for the confirmation and explanation in sublimelsp/LSP-ruff. 🙇

My problem is that the default for ruff-lsp is to use the ruff command installed with the ruf-lsp dependency, but since v0.0.18 it has been changed to use the ruff command if it exists in the environment. This is what I sent a pull request for. #52

A use case is to create a virtual environment as a project, and if the ruff command exists in that virtual environment, the ruff command will be used.

However, since v0.0.21, it has reverted back to its original behavior, which is why I commented.

I asked this question because I thought that something else (importStrategy) might need to be adjusted with this change.

@rchl
Copy link
Contributor

rchl commented Mar 13, 2023

Sublime Text takes a fairly simplistic approach of always running ruff-lsp and ruff from a separate environment because it's not really possible to activate project's own python environment reliably as in ST all windows share same process and thus system environment. So changing PATH (for example) in one window would affect all other windows and projects.

So the fact that it doesn't have issue with this change might be a factor of its limitations.

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.

3 participants