search

sublimelsp / LSP-pyright

Python support for Sublime's LSP plugin provided through microsoft/pyright.
MIT License
126 stars 13 forks source link

Venv resolution conflict with `./.venv` and `.python-version` file #331

Closed systemsoverload closed 3 months ago

systemsoverload commented 3 months ago

Expected:

Projects with a .venv should always have python loaded from this directory.

Actual:

Projects that have a .venv directory in the base of the project as well as a .python-version file in the base of project default to the command pyenv which python {version from .python-version} to resolve the venv and ignore the local venv.

This layout is the default behavior of Rye managed projects.

jfcherng commented 3 months ago

Fwiw, we basically ported https://github.com/fannheyward/coc-pyright/blob/6011417adce4098438a66883a2d03bea4a4d68d7/src/configSettings.ts#L47-L114. But I do think your proposal makes more sense.

catwell commented 3 months ago

I use Rye too and have the same issue. The main issue here is with this commit you do not check the return code of the subprocess anymore!

So before (with check_output) it raised an exception, which was caught, and it continued to the venv detection code.

Now it fails silently, so it triggers the return:

Screenshot from 2024-06-03 16-54-02

jfcherng commented 3 months ago

I use Rye too and have the same issue. The main issue here is with this commit you do not check the return code of the subprocess anymore!

That commit is even after this issue was filed.

jfcherng commented 3 months ago

Fwiw, we basically ported fannheyward/coc-pyright@6011417/src/configSettings.ts#L47-L114. But I do think your proposal makes more sense.

As I stated.

catwell commented 3 months ago

True! So my issue is slightly different since in my case it worked before this commit, i.e. before the last release (I use Rye but I do not have pyenv installed). But fixing this one would solve my issue too because it would use the .venv and never try to use pyenv.

jfcherng commented 3 months ago

I guess because https://github.com/fannheyward/coc-pyright/blob/6011417adce4098438a66883a2d03bea4a4d68d7/src/configSettings.ts#L54-L57 so coc-pyright works. But that most likely won't fit how ST users use ST.

jfcherng commented 3 months ago

I guess should add .venv folder as a special case (for better UX) as the highest priority.

I think it makes sense to search "arbitrary" subfolders to find a virtual env as the last help as in https://github.com/sublimelsp/LSP-pyright/blob/7d9269b0400eb2129e351f3a63f80f6704f4409c/plugin.py#L245-L251

The strategy coc-pyright used now is basically rely on env variables. Unfortunately that won't work well in ST since env variables are the same for all windows and last until ST gets closed. Plus, if a user (especially on Windows) used to click ST icon to start ST, the env-variable-way just won't work.

catwell commented 3 months ago

That works too! Actually a config like this is the first thing I looked for.

jfcherng commented 3 months ago

That works too! Actually a config like this is the first thing I looked for.

In the next release (probably the day after tomorrow), you can decide how venv is resolved.

By default, .venv at the project root (i.e., local_dot_venv) is preferred. https://github.com/sublimelsp/LSP-pyright/blob/bab20e63ab45e6eaf317fe07cef088f1e478e50e/LSP-pyright.sublime-settings#L20-L32

jfcherng commented 3 months ago

Update: I change the settings name from pyright.venv.strategies to venvStrategies.

https://github.com/sublimelsp/LSP-pyright/blob/301678f9d809922fe7cbbf7d4ee2dfb806d6f97f/LSP-pyright.sublime-settings#L13-L25

jfcherng commented 3 months ago

1.4.0 has been released and should be available within hours. You may need some ST restarts to make it work since new lib dependencies are added.