-
Notifications
You must be signed in to change notification settings - Fork 177
Support for Wrokspace Folders + embedded projects #178
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
base: main
Are you sure you want to change the base?
Conversation
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.
…mbedded in another + unit tests
| describe('M._get_buf_root()):', function() | ||
| it("testing M._get_buf_root() returns correct buf_root for embedded projects", function() | ||
| local outter_project = "/Users/kkrime/outter_project" | ||
| local inner_project = "/Users/kkrime/outter_project/inner_project" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unsure if it really matters, but it seems some more universal paths like /tmp/outer_project could work better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It really doesn't make any difference, you can change it you feel strongly about it, no problem with me
72c6c5b to
c11869a
Compare
c11869a to
a6a0763
Compare
|
@kkrime Quick question: I tried this plugin, via LazyVim, but the list of recent projects remains empty. Is it broken and no longer maintained? Are your PRs being merged here or in another fork? Thank you! |
|
@magnusriga I'm not sure what's happening with this project tbh, it hasn't been touched by the maintainer in a while, it's looking like it's been abandoned. I'm not sure what you mean by |
@kkrime Thank you for the quick reply. I tried to run Does that table fill up on your end, when you open files and LSP clients attach? |
|
@magnusriga Ya, it works for me, did you configure |
|
@kkrime I am using {
"kkrime/project.nvim",
opts = { },
event = "VeryLazy",
config = function(_, opts)
require("project_nvim").setup(opts)
end,
}Does it need any other config? Edit: It is supposed to set |
|
@magnusriga try coping my config https://github.com/kkrime/home/blob/02bf84d93431d4345e0bf07f3a5f6282df9cb257/.config/nvim/lua/plugins/telescope.lua#L10 Also make sure your lsp is running |
|
@kkrime Thanks for helping me. I found the issue:
Hope that is helpful to others in the future. Thanks again for being so responsive. |
|
It looks like the maintainer hasn't been active for two years. I created a fork I'm actively maintaining and would love to have your enhancement. That is, of course, if you want to. |
@magnusriga Lazy-Loading seems to be fixed in my fork. |
Hey @DrKJeff16, sure feel free to use my code, no worries
I'm not sure what the issue yoru facing is, but here's how I run it; I would remove Good luck 👍 |
@kkrime
Funny thing is I just did some new refactoring today that seemingly fixes that behaviour. Not intentionally, lol. I'm honestly not sure yet about what piece of code is involved in this. Essentially, |
|
Hey guys, I can't remember what I ended up doing here. I think I moved to the same solution that LazyVim uses, which might have been some snacks.nvim submodule or similar. I will check back in if it stops working. Thanks for the effort and input 🥇🚀 |
| local buf_name_to_file_path_map = {} | ||
|
|
||
| vim.api.nvim_create_autocmd("BufDelete", { | ||
| pattern = "*", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's really convenient to use augroups:
local group = vim.api.nvim_create_augroup("ProjectTest", { clear = true })
vim.api.nvim_create_autocmd("BufDelete", {
group = group,
pattern = "*",
--- ...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would the value in using augroups be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly bundling augroups into categories. Makes them easier to find (i.e. when using vim.api.nvim_exec_autocmds() and so on.
For example if you want to run an autocmd through the cmdline,
you don't want to run ALL the, say, "BufAdd" autocmds. You might want to run just the ones related to, let's say, gitsigns as an arbitrary example.
| M.last_project = nil | ||
|
|
||
| ---@alias buf_name string this is the name of the buffer (the path/filename that is opened in the buffer) | ||
| ---@alias buf_file_location string this is the buf_name minus the filename (just the path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not document the values that use this alias instead of "wrapping" an alias type around string? I get what you want to do but in the long run it might confuse people.
|
|
||
| --- regex is relatively expensive, so store buf_file_path in map | ||
| ---@type table<buf_name, buf_file_location> | ||
| local buf_name_to_file_path_map = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having ad-hoc types, in my personal opinion, could be an issue for understanding code.
| local buf_name_to_file_path_map = {} | ||
|
|
||
| vim.api.nvim_create_autocmd("BufDelete", { | ||
| pattern = "*", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly bundling augroups into categories. Makes them easier to find (i.e. when using vim.api.nvim_exec_autocmds() and so on.
For example if you want to run an autocmd through the cmdline,
you don't want to run ALL the, say, "BufAdd" autocmds. You might want to run just the ones related to, let's say, gitsigns as an arbitrary example.
| root_dir = folder_name | ||
| elseif #folder_name > #root_dir then | ||
| root_dir = folder_name | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest two alternatives:
Simple Conditional
if not (root_dir and #folder_name <= #root_dir) then
root_dir = folder_name
endUsing Ternary Operators
root_dir = not (root_dir and #folder_name <= #root_dir) and folder_name or root_dirThis one acts like a ternary operator.1
The reason I'm doing not (root_dir and #folder_name <= #root_dir) is a simple grouping of logical conditions.
Expanding it would look like not root_dir or not #folder_name <= #root_dir.
The inverse of not #folder_name <= #root_dir is #folder_name > #root_dir.
Footnotes
|
|
||
| -- Fall back to root_dir | ||
| return client.config.root_dir | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative is to kill the if root_dir then code block and use this return statement:
return not (root_dir) and client_config.root_dir or root_dir
end| local clients = vim.lsp.buf_get_clients() | ||
| local buf_ft = vim.api.nvim_get_option_value('filetype', { buf = 0 }) | ||
| local buf_name = vim.api.nvim_buf_get_name(0) | ||
| local clients = vim.lsp.get_clients({ bufnr = 0 }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't recommend using 0 as bufnr inside a function. An alternative:
local bufnr = vim.api.nvim_get_current_buf()
local buf_ft = vim.api.nvim_get_option_value('filetype', { buf = bufnr })
local buf_name = vim.api.nvim_buf_get_name(bufnr)
local clients = vim.lsp.get_clients({ bufnr = bufnr })| if not vim.tbl_contains(config.options.ignore_lsp, client.name) then | ||
| return client.config.root_dir, client.name | ||
| local lsp_root = M._lsp_get_buf_root(client, buf_name) | ||
| return lsp_root, client.name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion:
return M._lsp_get_buf_root(client, buf_name), client.name| if client.workspace_folders then | ||
| for _, workspace_folder in pairs(client.workspace_folders) do | ||
| local folder_name = vim.uri_to_fname(workspace_folder.uri) | ||
| if folder_name and vim.startswith(buf_file_path, folder_name) then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest inverting this conditional to reduce the complexity:
if not (folder_name or vim.startswith(buf_file_path, folder_name)) then
--- Fail case
end
Ya sure, I noticed you've done a lot of refactoring, so I might have a few questions, but ya, I'll get on to it when I get time |
No description provided.