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

Update pyright.lua to use cwd() if no python config files are present. #2697

Merged
merged 1 commit into from
Jul 13, 2023

Conversation

hacker-DOM
Copy link
Contributor

This behavior is more natural and the default in VS Code: microsoft/pyright#5407 (comment).

@glepnir
Copy link
Member

glepnir commented Jul 4, 2023

there has single_file_mode = true it will use cwd .

@hacker-DOM
Copy link
Contributor Author

hacker-DOM commented Jul 4, 2023

there has single_file_mode = true it will use cwd .

I'm not 100% this covers my use case. My use case is:

a directory with stuff like

tests/
  f.py 
pytypes/
  a.py

In tests/f.py, I would like to do from pytypes.a import A

In Vs Code this works. In Nvim I currently have to create a pyproject.toml tool.pyright.include = ['.']. This PR changes the root dir to default to cwd if no config file is found among parents (just as in vs code).

@glepnir
Copy link
Member

glepnir commented Jul 4, 2023

if in tests folder open f.py the root will be tests i guess. tests and pytypes both in a project?

@hacker-DOM
Copy link
Contributor Author

if in tests folder open f.py the root will be tests i guess. tests and pytypes both in a project?

no no, my cwd is still same, I am not cding anywhere.

Yeah we have pytypes as well as tests.

@glepnir
Copy link
Member

glepnir commented Jul 4, 2023

hmm. then change your commit message force push. fix(pyright): fallback to cwd when root config not found

@glepnir glepnir merged commit bd14d2f into neovim:master Jul 13, 2023
8 of 9 checks passed
@anthony-S93
Copy link
Contributor

anthony-S93 commented Jul 21, 2023

This is a bad idea. Like I mentioned in #2728 , this PR disables single-file mode entirely. You are forcing the root directory to always fallback to Neovim's current working directory even though some files are meant to be edited with the LSP launched in single-file mode without any root directory set.

This behavior is more natural and the default in VS Code

Just because a behavior is the default for VS Code does not mean it is appropriate for the workflow of Neovim. Let me give you an example of why this PR is such a bad idea.

Let's say I'm currently in my home directory when I launch neovim from the terminal. This will essentially set Neovim's cwd to my home directory. After neovim launches, I open a file script.py from inside neovim :e /path/to/script.py. script.py is not a component of some grand software repository. It is a utility script that performs some routine task. As such, it will sit inside a directory without any root markers. And now because of this PR, script.py will never open in single-file mode. Since neovim's cwd is my home directory, nvim-lspconfig opens pyright with the root directory set to my home directory. But what if my home directory is a huge directory with hundreds of files? This causes significant deterioration in terms of LSP performance. Since the root is set to my home directory, pyright will have to analyse the entire directory for no reason at all. script.py is not even part of any repo. It is meant to be edited in single-file mode. On my system, it took a few seconds for Pyright to even load.

Here's a video illustrating the workflow I just described. Pyright's root directory is set to neovim's cwd even though the file is meant to be edited in single-file mode.

NoSingleFileMode.mp4

Neovim is not VSCode
How does one uses VSCode? You launch VSCode, select a working directory from the side panel, and then start editing the files inside the working directory you just selected. But that's not how you use neovim. In neovim, you sometimes want to open files that are outside of neovim's cwd.

Neovim is not VSCode. They are both different editors designed for different workflows.

@glepnir
Do I have permission to revert the changes in this PR? I have already tested the plugin without this PR. Pyright's performance issue went away after I removed this PR (duh).

@glepnir
Copy link
Member

glepnir commented Jul 21, 2023

lsp was born based on vscode. Other editors benefit from this agreement. So you can see that most editors use vscode as a reference when implementing lsp. The problem is due to a difference in the design of the editors. There is no guarantee that the behavior will be 100% consistent with vscode.

pr breaks the single-file mode. Because the root directory of the cwd will be generated anyway. pyright will retrieve all pyhton files under cwd. My suggestion is to leave it as it is (remove cwd). If @hacker-DOM you need to use cwd, modify it in your local pyright. (lspconfig provides a basic server configuration. Meet the needs of most people. For individual usage scenarios. Local modifications are more appropriate)

@anthony-S93
Copy link
Contributor

anthony-S93 commented Jul 21, 2023

lsp was born based on vscode. Other editors benefit from this agreement. So you can see that most editors use vscode as a reference when implementing lsp. The problem is due to a difference in the design of the editors. There is no guarantee that the behavior will be 100% consistent with vscode.

pr breaks the single-file mode. Because the root directory of the cwd will be generated anyway. pyright will retrieve all pyhton files under cwd. My suggestion is to leave it as it is (remove cwd). If you need to use cwd, modify it in your local pyright. (lspconfig provides a basic server configuration. Meet the needs of most people. For individual usage scenarios. Local modifications are more appropriate)

@glepnir
Huh? So are you agreeing with me that this PR should be reverted? I'm a bit confused here. This PR was merged last week.

@glepnir
Copy link
Member

glepnir commented Jul 21, 2023

My suggestion is to leave it as it is (remove cwd).

revert or remove .

@anthony-S93
Copy link
Contributor

My suggestion is to leave it as it is (remove cwd).

revert or remove .

Creating a PR to fix this now.

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