fix(setup): not really detecting external clients

[hoofcushion]

Two Problem:

• tbl == {} is not the right way to tell if a table is empty in Lua, which makes the external_client meaningless.

• If two client starts in the same event loop, it will start zk twice. e.g. when user try to use filetype autocmd to start zk and zk-nvim together.

Two Solution:

• Use next(tbl)==nil or vim.tbl_isempty(tbl) instead.

• Use vim.schedule to prevent multiple client starts in the same event loop.

[tjex]

Great, thanks for this!! I've since found a bunch of other spots where this occurs, and would go about changing those. So I'm wondering, why did you choose vim.tbl_isempty() instead of next()?

It's better to use lua stdlib wherever possible or (for performance, future proof, idiomatics)?

[hoofcushion]

It depends on personal preference. tbl_isempty has clear semantics, and it is equivalent to next. And using tbl_isempty here will not cause performance issue. I prefer using next too.

[hoofcushion]

vim.tbl_isempty API may change when Neovim update, next is better.

[tjex]

vim.tbl_isempty API may change when Neovim update, next is better.

yeah, I was also thinking in this direction. But being unsure whether there was some other magic happening behind the vim class.

Then I will go through and change all the other instances of it too and push to your branch. May as well chop them all down while we're at it.

[tjex]

ah, that was all of them 😅 Great haha

[hoofcushion]

It's necessary for me to use vim.schedule. otherwise, it will start zk lsp twice/

[tjex]

Ahhh. It looks like you've installed zk with mason. This is why. zk has its own lsp built in. And looking through the readme both on zk-nvim and zk, this is really not clear enough 😩

So zk lsp from mason is being attached as well as zk's built in lsp... You just need to uninstall zk via Mason. Then you'll be set.

[hoofcushion]

The ideal situation is zk-nvim detect the existed Zk LSP client instance and try to attach to it. So zk-nvim should wait and see if any client is created.

[hoofcushion]

I refactored the code, vim.schedule is removed, as I find out turn off the autoattach option of zk-nvim will avoid the conflict to use mason & lspconfig when launch zk lsp.

[hoofcushion]

But I think it's better to not set anything manually to use mason & lspconfig and zk-nvim together.

[tjex]

I think this is becoming overcomplicated. There are two options on the user's end:

1. Use zk cli -> do not install zk lsp via Mason / manually
2. Do not use zk cli -> install zk lsp via Mason / manually

I think that instruction is simpler for the user and simpler for code maintenance.

Thanks for the consideration and effort though!

But I'll close for now as, despite the lsp being fired twice in your case (and no doubt for other too), this will not create any adverse behaviour.

I refactored the code, vim.schedule is removed, as I find out turn off the autoattach option of zk-nvim will avoid the conflict to use mason & lspconfig when launch zk lsp.

Again, the solution is to uninstall Mason's zk install.

[hoofcushion]

There still has problem with tbl == {}, if it tries to get external client, then it has to get it welly.

[hoofcushion]

As I said, when use mason, disable autoattach for zk-nvim will fix the problem 2, since mason & lspconfig will help to set up lsp for zk-nvim. It's better for user to not worry about which zk to use.

[tjex]

I understand. But in the case of the user needing to be aware and make a decision; they still need to include the autoattach = false in their config for zk-nvim specifically. So they have to be "extra careful" and make an extra effort anyway.

This means we would change a lot of the code to account for this, and the user would still have to know a finer detail of the setup process and apply that change to their own config. Particularly for people getting to grips with neovim, telling them to implement opt.lsp.autoattach = false to their config is more complicated than saying what is now written in the readme (i.e., zk cli = no need for zk lsp installation).

If you want to make a new pr that addresses the tbl == {} then I'd be happy to merge that (or go about reverting and cleaning up all the git history in this pr, but that is probably more effort) 🙏

[hoofcushion]

done