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

fix(lsp): prioritise workspace_folders for root directory detection #147

Closed
wants to merge 1 commit into from

Conversation

mrcjkb
Copy link

@mrcjkb mrcjkb commented Feb 19, 2024

Some LSP clients can have more than one workspace folder, which can be added via
workspace/didChangeWorkspaceFolders.

This is especially useful for resource-hungry clients, like rust-analyzer or haskell-language-server, in order to avoid spawning multiple clients.

Using lsp.Client.config.root_dir as the source of truth results in this plugin detecting the wrong project root in this scenario.

This PR fixes that, by first searching through the lsp.Client.workspace_folders,
and then falling back to the config.root_dir if no matching workspace folder is found.

@mrcjkb mrcjkb force-pushed the use-workspace-root branch 2 times, most recently from 390f8b8 to ab1c697 Compare February 19, 2024 21:28
Some LSP clients can have more than one workspace folder, which
can be added via
[`workspace/didChangeWorkspaceFolders`](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workspace_didChangeWorkspaceFolders).

This is especially useful for resource-hungry clients, like
rust-analyzer or haskell-language-server, in order to avoid spawning
multiple clients.

Using `lsp.Client.config.root_dir` as the source of truth results
in this plugin detecting the wrong project root in this scenario.

This PR fixes that, by first searching through the
`lsp.Client.workspace_folder`s,
and then falling back to the `config.root_dir` if no matching
workspace folder is found.
@mrcjkb mrcjkb force-pushed the use-workspace-root branch from ab1c697 to 008fb3d Compare May 27, 2024 14:36
@kkrime
Copy link

kkrime commented Nov 23, 2024

@mrcjkb I implemented something smiliar before I found out about this PR; #172

Quick questoin, how would your code deal with embedded projects?

As in a project inside another project, would it take the inner project as the root or the outter? Beucase both would be in client.workspace_folders?

@mrcjkb
Copy link
Author

mrcjkb commented Nov 23, 2024

Oh wow I forgot about this PR. It's quite a while ago and I don't use project.nvim anymore (because I use tmux sessions instead).
Looking at the code, it just takes the first one it finds.
If #172 solves the problem this PR solves, then I'd suggest just to close this PR.
I had a brief look, but it's so long ago that I don't remember exactly what the problem was.

By the way, I noticed you're using goto statements. In case you're not aware: That limits this plugin to Neovim built with luajit. On some systems (e.g. termux on Android), Neovim is built with Lua 5.1, which doesn't support goto statements.

@kkrime
Copy link

kkrime commented Nov 23, 2024

@mrcjkb I think your code is neater and more conciuse, so if it's cool with you, I might just extend what you've already built?

Thanks for the heads up about the goto statement and lua (I'm really new to the world of lua and nvim plugins)

Probably worth letting @ahmedkhalf know this and #172 are duplicates

@kkrime
Copy link

kkrime commented Jan 7, 2025

@mrcjkb FYI - I extended your work to include code to cover a corner case (embedded projects) - I left in your original comment https://github.com/ahmedkhalf/project.nvim/pull/178/commits

@ahmedkhalf please use this for Workspace Folders Support: #178, I added unit tests too.

@mrcjkb
Copy link
Author

mrcjkb commented Jan 7, 2025

@mrcjkb FYI - I extended your work to include code to cover a corner case (embedded projects) - I left in your original comment https://github.com/ahmedkhalf/project.nvim/pull/178/commits

@ahmedkhalf please use this for Workspace Folders Support: #178, I added unit tests too.

awesome, thanks for taking over 🙏

@mrcjkb mrcjkb closed this Jan 7, 2025
@mrcjkb mrcjkb deleted the use-workspace-root branch January 7, 2025 06:18
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.

2 participants