feat(lsp): dynamic registration support for onTypeFormatting #35578
Conversation
| @@ -925,6 +925,11 @@ function Client:_register(registrations) | |||
| local method = reg.method | |||
| if method == ms.workspace_didChangeWatchedFiles then | |||
| lsp._watchfiles.register(reg, self.id) | |||
| elseif method == ms.textDocument_onTypeFormatting then | |||
| -- if we enabled OTF for this client before the capability was registered, reattach... | |||
There was a problem hiding this comment.
I don't think this is possible because when attaching we check that the clients supports the method, and until the registration is made then supports_method would return false there.
There was a problem hiding this comment.
In fact I don't think you need to do anything special here.
There was a problem hiding this comment.
I see- without anything here though, if we enable OTF globally and then have a client attach with dynamic registration, the feature will still have to be manually enabled for the client after it registers. Is this alright?
(I should mention the test included in this PR doesn't pass in the current state)
There was a problem hiding this comment.
Also, in my testing, when I enabled OTF in this line it did get the test to pass. Maybe since it is after the self:_register_dynamic(registrations) line, supports_method is able to return true? Not confident about this...
There was a problem hiding this comment.
I would move the logic of enabling OTF to the registerCapability method handler. This is where I added the code for document color: https://github.com/MariaSolOs/neovim/blob/f311c96973a561ba5e664f46e758a97fd10acdcb/runtime/lua/vim/lsp/handlers.lua#L153
There was a problem hiding this comment.
Thanks, that makes sense.
Also, I think OTF needs to differentiate between enable() and attach() in order for dynamic registration to work properly. This is because we need to be able to attach()/detach() (as (un)registrations come in) without affecting the user's preferred enablement state. Is this possible with lsp._capability? cc @ofseed
There was a problem hiding this comment.
... actually, that is unless we opt to settle for scratch that, we definitely need this, otherwise we can't detach without disablingif foo_is_enabled() then foo_enable() end in the code base, which reads poorly but requires less code changes
There was a problem hiding this comment.
Yes, separating the enable/disable logic from attach/detach is the main goal of #35018. This requires 1. using vim.lsp._capabilit.enable() to control enable/disable, and 2. extending these state handlers, which exist independently on a per-buffer basis, from vim.lsp.Capability. If anything needs to be done on client attach, modify Capability:on_attach.
Draft for now. I have some questions, e.g.: is there a preferred way to handle
enable_foo()vs.attach_foo()? Should they be one and the same? I ask because (you can see the first commented-out code chunk) that it can be a bit confusing to callif foo_is_enabled() then foo_enable() end, but it's not that big of a deal.