Skip to content

Feature: migrate builtin commands #3734

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

Merged

Conversation

TheRealLorenz
Copy link
Contributor

@TheRealLorenz TheRealLorenz commented Apr 16, 2025

fix #3714
fix #2216

Things to note:

  • LspStart starts the selected LSP and attaches to every buffer which has the right filetype. Should I only attach it to the current buffer?

  • LspStop stops the selected LSP.

  • LspRestart restart the selected LSPs, but I don't get how and why it used a timer.

  • Should we add LspEnable? Maybe in a future PR.

EDIT:

Should I add checks for neovim 0.11?

Something like:

  if not has_eleven then
    error('nvim-lspconfig 2.x only supports vim.lsp.config. If you dont want to migrate, pin to nvim-lspconfig 1.x')
  end

@seblyng
Copy link
Contributor

seblyng commented Apr 18, 2025

I don't think LspStart and LspRestart will work when root_dir is a function with this. I think you must do something like this:

if type(config.root_dir) == 'function' then
  ---@param root_dir string
  config.root_dir(bufnr, function(root_dir)
    config.root_dir = root_dir
    vim.schedule(function()
      vim.lsp.start(config, {
        bufnr = bufnr,
        reuse_client = config.reuse_client,
        _root_markers = config.root_markers,
      })
    end)
  end)
else
  vim.lsp.start(config, {
    bufnr = bufnr,
    reuse_client = config.reuse_client,
    _root_markers = config.root_markers,
  })
end

Inspiration taken from neovim source code, in lsp_enable_callback function in vim/lsp.lua

@TheRealLorenz

This comment was marked as resolved.

@TheRealLorenz
Copy link
Contributor Author

Opened a PR at neovim/neovim#33578 to make :LspStart less ugly.

@TheRealLorenz
Copy link
Contributor Author

TheRealLorenz commented Apr 23, 2025

@justinmk On further thinking, probably these commands do not suit vim.lsp quite well (at least semantically).

We could use LspEnable (with optional ++now), LspDisable, LspStart and LspStop "systemctl-like"

EDIT: or we could have enable + start as LspStart and disable + stop as LspStop

@TheRealLorenz
Copy link
Contributor Author

What's the current situation of this after neovim/neovim#33702? @justinmk

It should simplify logic as:

:LspStart xxx as vim.lsp.enable(xxx)
:LspStop xxx as vim.lsp.enable(xxx, false)
:LspRestart xxx as vim.lsp.enable(xxx, false); vim.lsp.enable(xxx)

Probably it would be useful to rename the commands when (and if) they're upstreamed to neovim core.

@justinmk
Copy link
Member

justinmk commented May 1, 2025

It should simplify logic as:

yes, exactly 👍

rename the commands when (and if) they're upstreamed to neovim core.

yeah, most likely the upstreamed form will be :Lsp <subcommand>

@justinmk
Copy link
Member

justinmk commented May 1, 2025

It probably makes sense to add a if vim.fn.has('nvim-0.11.2') == 1 section starting here:

api.nvim_create_user_command('LspInfo', ':checkhealth vim.lsp', { desc = 'Alias to `:checkhealth vim.lsp`' })

which defines the commands for Nvim 0.11.2+, and then return early. Then the old code below that, can just stay as it is.

@TheRealLorenz TheRealLorenz force-pushed the feature/migrate-builtin-commands branch from 63962b4 to bb259b5 Compare May 1, 2025 10:33
@TheRealLorenz
Copy link
Contributor Author

Is there a way to provide completion for :LspStart ? I didn't find one as the configs are lazily resolved on request if I'm correct.

Copy link
Member

@justinmk justinmk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like a good starting point, we can improve it if needed

@justinmk
Copy link
Member

justinmk commented May 1, 2025

provide completion for :LspStart ? I didn't find one as the configs are lazily resolved on request if I'm correct.

oh, we could copy the logic from here: https://github.com/neovim/neovim/blob/97a6259442526e33a73201557b1cb74ccdb64eef/runtime/lua/vim/lsp.lua#L443-L454

@TheRealLorenz
Copy link
Contributor Author

provide completion for :LspStart ? I didn't find one as the configs are lazily resolved on request if I'm correct.

oh, we could copy the logic from here: https://github.com/neovim/neovim/blob/97a6259442526e33a73201557b1cb74ccdb64eef/runtime/lua/vim/lsp.lua#L443-L454

That would be fine for getting configs under lsp/ but wouldn't cover configs defined by the user in his dotfiles as vim.lsp.config('foo', {}), am I right?

@justinmk
Copy link
Member

justinmk commented May 1, 2025

wouldn't cover configs defined by the user in his dotfiles as vim.lsp.config('foo', {}), am I right?

right, but we can think about that later.

@TheRealLorenz TheRealLorenz force-pushed the feature/migrate-builtin-commands branch 2 times, most recently from 652462b to d846349 Compare May 1, 2025 13:07
@TheRealLorenz TheRealLorenz force-pushed the feature/migrate-builtin-commands branch from d846349 to 950e3d6 Compare May 1, 2025 13:17
@TheRealLorenz TheRealLorenz marked this pull request as ready for review May 1, 2025 13:17
@TheRealLorenz TheRealLorenz requested a review from glepnir as a code owner May 1, 2025 13:17
@justinmk justinmk merged commit e4d1c8b into neovim:master May 4, 2025
10 checks passed
@TheRealLorenz TheRealLorenz deleted the feature/migrate-builtin-commands branch May 9, 2025 22:09
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.

migrate :Lsp commands to vim.lsp.config LspStop should remove related autocmds
3 participants