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

feat(config): Allow overriding the root_dir #402

Merged
merged 4 commits into from
May 27, 2024
Merged

Conversation

bgw
Copy link
Contributor

@bgw bgw commented May 19, 2024

I'm working in a repository with git submodules. Each submodule has a Cargo.toml with a workspace definition. The parent repository's workspace is a carefully-configured superset of all of the the child repository workspaces.

When working in the top-level repository, I always want rust-analyzer to use the top-level repository as the root, so that I don't end up with multiple overlapping instances of rust-analyzer.

Cargo doesn't really support nested workspaces, and its default behavior is to find the nearest/innermost workspace, which is not what I want.

This is a niche use-case, but it would be nice to have a way to override the default behavior.

This is loosely modeled after nvim-lspconfig's root_dir option: https://github.com/neovim/nvim-lspconfig/blob/94513a5b246cf32a8f87ca714af50911df63351c/CONTRIBUTING.md#adding-a-server-to-lspconfig

  • Why get_root_dir and not root_dir? RustaceanLspClientConfig is a subtype of vim.lsp.ClientConfig. The vim.lsp.ClientConfig type has a conflicting root_dir field that's typed as a string (not a function).

  • Why does it not also take bufnr, like nvim-lspconfig's API? There's no way to get the bufnr value in the neotest adapter. It's also probably not needed in most cases.

Testing

Run tests:

nix build .#checks.x86_64-linux.nvim-stable-tests --print-build-logs

Configure a get_root_dir function to return a hardcoded value, and inspect the results with :LspInfo.

 {
   "mrcjkb/rustaceanvim",
   dir = "/home/bgw/rustaceanvim",
   opts = {
     server = {
       get_root_dir = function()
         return "/home/bgw/..."  -- hardcoded value
       end,
     },
   },
 },

Configure neotest, and use the neotest summary to see the appropriate tests listed for my hardcoded root.

Copy link
Contributor

Review Checklist

Does this PR follow the Contribution Guidelines? Following is a partial checklist:

Proper conventional commit scoping:

  • For example, fix(lsp): some lsp-related bugfix

  • Pull request title has the appropriate conventional commit prefix.

If applicable:

  • Tested
    • Tests have been added.
    • Tested manually (Steps to reproduce in PR description).
  • Updated documentation.
  • Updated CHANGELOG.md

@mrcjkb
Copy link
Owner

mrcjkb commented May 20, 2024

Hey 👋

Thanks for the PR 🙏

Why get_root_dir and not root_dir? RustaceanLspClientConfig is a subtype of vim.lsp.ClientConfig. The vim.lsp.ClientConfig type has a conflicting root_dir field that's typed as a string (not a function).

🤔 Is it possible to override the root_dir field with LuaCATS annotation, so that it's a string | fun(config: RustaceanLspClientConfig, filename: string):string|nil?

If the lua-ls linter complains, it ought to be possible to disable the warning for the next line (I do that in some places).

@bgw
Copy link
Contributor Author

bgw commented May 22, 2024

Thanks for taking a look. I've updated the PR.

Renaming get_root_dir to root_dir "just works". I'm guessing vim.tbl_deep_extend('force', {}, client_config) is not properly/strongly typed. My attempt to avoid it was less because I technically couldn't, and more because I felt like it would be less ugly to pick a name than override the type. If you're okay with it, I am.

Type checks pass with

nix build .#checks.x86_64-linux.type-check-stable --print-build-logs

so that it's a string | fun(config: RustaceanLspClientConfig, filename: string):string|nil?

I don't think we should accept a simple string for this value (instead of a function), as (outside of contrived tests like I'm running) you'd never want a fixed value for the root_dir.

lua/rustaceanvim/config/init.lua Outdated Show resolved Hide resolved
lua/rustaceanvim/config/internal.lua Show resolved Hide resolved
lua/rustaceanvim/config/internal.lua Show resolved Hide resolved
@bgw
Copy link
Contributor Author

bgw commented May 22, 2024

Thanks for the thorough and thoughtful review. I'll revise this PR with the suggested changes soon.

bgw added 3 commits May 26, 2024 21:20
I'm working in a repository with git submodules, which leads to nested
cargo workspaces. When working in the top-level repository, I always
want rust-analyzer to use the top-level repository as the root, so that
I don't end up with multiple overlapping instances of rust-analyzer.

This is a niche use-case, but it would be nice to have a way to override
the default behavior.

This is loosely modeled after nvim-lspconfig's `root_dir` option:
https://github.com/neovim/nvim-lspconfig/blob/94513a5b246cf32a8f87ca714af50911df63351c/CONTRIBUTING.md#adding-a-server-to-lspconfig

- **Why `get_root_dir` and not `root_dir`?** `RustaceanLspClientConfig`
  is a subtype of `vim.lsp.ClientConfig`. This is taken advantage of
  when constructing the config for each individual client. The
  `vim.lsp.ClientConfig` type has a conflicting `root_dir` field that's
  typed as a string (not a function).

- **Why does it not take `bufnr`, like nvim-lspconfig's API?** There's
  no way to get the `bufnr` value in the neotest adapter. It's also
  probably not needed in most cases.
- Add a validator to config/check.lua
- Accept a string for `root_dir`
- Don't mention the default implementation as the private
  `rustaceanvim.cargo.get_root_dir` function.
- Pass an extra "default" argument with the default implementation.
- Update documentation/comments where relevant.
@bgw
Copy link
Contributor Author

bgw commented May 27, 2024

I've updated the PR with the suggested changes! Let me know if there's any further changes I should make.

@bgw bgw requested a review from mrcjkb May 27, 2024 04:31
@mrcjkb mrcjkb merged commit 691901d into mrcjkb:master May 27, 2024
6 checks passed
@mrcjkb
Copy link
Owner

mrcjkb commented May 27, 2024

Thanks again 🙏

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