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

refactor: use vim.lsp.start instead of vim.lsp.start_client #3450

Merged
merged 2 commits into from
Nov 24, 2024

Conversation

MariaSolOs
Copy link
Member

This PR replaces the usage of vim.lsp.start_client by vim.lsp.start, delegating the logic of client reuse to the editor.

Also adding support for a silent option to prevent error notifications on init.

@MariaSolOs MariaSolOs requested a review from glepnir as a code owner November 24, 2024 03:57
@MariaSolOs MariaSolOs requested review from lewis6991 and gpanders and removed request for glepnir November 24, 2024 03:58
@glepnir
Copy link
Member

glepnir commented Nov 24, 2024

minimum compatible version is 0.9 now unless minimum bump to 0.10 but there still have 0.9 user

@MariaSolOs
Copy link
Member Author

minimum compatible version is 0.9 now unless minimum bump to 0.10 but there still have 0.9 user

Correct me if I'm wrong but vim.lsp.start is available in 0.9?

@glepnir
Copy link
Member

glepnir commented Nov 24, 2024

yes. i remember wrong version just tried on 0.9

@MariaSolOs
Copy link
Member Author

yes. i remember wrong version just tried on 0.9

Thanks for checking!

--- @param root_dir string
--- @param client vim.lsp.Client
--- @param single_file boolean
function M:_attach_after_client_initialized(bufnr, new_config, root_dir, client, single_file)
Copy link
Member

@glepnir glepnir Nov 24, 2024

Choose a reason for hiding this comment

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

When neovim start with reads the session it may contain multiple projects. This needs to be checked after the first server start.

I think this PR is better to simply replace vim.lsp.start_client by vim.lsp.start rather than refactor other functions. our goal is more to add support in the core. If the overall work is good now, it is OK to leave it this way without too much change.

Copy link
Member Author

Choose a reason for hiding this comment

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

The logic of _attach_or_spawn is:

  1. Call lsp.buf_attach_client (which neovim does inside vim.lsp.start so no need to do it here)
  2. Add the client to the _clients cache
  3. Send a workspace/didChangeWorkspaceFolders notification if the server supports it.

The implementation in this PR keeps 2 and 3, event after the server is first initialized. Or are you referring to something else?

Copy link
Member

@glepnir glepnir Nov 24, 2024

Choose a reason for hiding this comment

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

vsplit has two buffers in two projects. run mksession and then starts neovim with -S session.vim. The first buffer triggers FileType to start a server. Note that this is an asynchronous ,server that needs initialization time. At this point you are not sure if the server supports workspacefolder. But the second buffer has triggered FileType again. The embed process is processed continuously. So the second buffer should wait for the server started by the first buffer to complete initialization and check whether to connect to it or start a new one.

that's why there have a timer check client.intialized if currently refactor can handle it as expect. ignore me.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see. I've replaced the code using the timer by adding this hook to on_init.

Copy link
Member

@glepnir glepnir Nov 24, 2024

Choose a reason for hiding this comment

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

if I remember correctly, this is a hook for modifying the parameters of the server itself. It runs after the root judgment. The problem is that if the server has not initialized the second buffer, the same event loop will run the launch function. Can we know that the first server started supports workspacefolder? Shouldn't there be a synchronous waiting problem here? I have forgotten some of the code here. I simply tested this Pr code and it works as I expected. But I still don't understand the logic here.

@glepnir glepnir changed the title feat: use vim.lsp.start instead of vim.lsp.start_client refactor: use vim.lsp.start instead of vim.lsp.start_client Nov 24, 2024
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.

Thank you, each incremental code reduction like this gets us closer to upstreaming to core. Nice naming improvements too.

@justinmk justinmk merged commit dafd61d into neovim:master Nov 24, 2024
11 checks passed
@MariaSolOs MariaSolOs deleted the start branch November 24, 2024 14:53
@MariaSolOs
Copy link
Member Author

Thank you, each incremental code reduction like this gets us closer to upstreaming to core. Nice naming improvements too.

That’s the goal indeed! And the name changes were mostly because I had to stare at the code for a while to understand what it was doing 😅

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