Use handlers instead of callbacks #399

Iron-E · 2020-11-13T09:04:52Z

The recent changes in Neovim head cause loading this file to create an error. These changes have resolved that on my machine.

Note: neovim/neovim#12655

lithammer · 2020-11-13T12:51:46Z

I suspect you have to fix the obsolete callbacks references in lua/nvim_lsp/util.lua and lua/nvim_lsp/configs.lua as well...? It might work without obvious errors, but I have a feeling some functionality is just going into a void since they're never called anymore.

Iron-E · 2020-11-13T19:03:18Z

You're right! I missed those. Gonna change them and commit.

Iron-E · 2020-11-13T19:16:06Z

Changed the rest of them.

teto · 2020-11-14T17:23:45Z

@tjdevries

tjdevries · 2020-11-14T18:32:48Z

lua/nvim_lsp/configs.lua

-        assert(lsp.callbacks, "Update to Nvim HEAD. This is an incompatible interface.")
-        assert(lsp.callbacks["window/logMessage"], "Callback for window/logMessage notification is not defined")
-        lsp.callbacks["window/logMessage"](err, method, params, client_id)
+        assert(lsp.handlers, "Update to Nvim HEAD. This is an incompatible interface.")


I think we should assert(not lsp.callbacks, ... with an informative message and a help section to look at .

Once we do that, we can merge. ("Update to Nvim HEAD" doesn't tell people what / how they need to fix anything)

Did the most recent commit address this well?

tjdevries · 2020-11-14T19:24:14Z

lua/nvim_lsp/configs.lua

-        assert(lsp.callbacks, "Update to Nvim HEAD. This is an incompatible interface.")
-        assert(lsp.callbacks["window/logMessage"], "Callback for window/logMessage notification is not defined")
-        lsp.callbacks["window/logMessage"](err, method, params, client_id)
+        assert(not lsp.callbacks, "lsp.callbacks has been deprecated. See here for more: https://github.com/nvim-lua/diagnostic-nvim/issues/73")


I think this should be moved out of the inner function. This should be on line 36 I think (where we add the handlers).

We don't need to check this every time.

Also, it should reference this PR: neovim/neovim#12655 Which is the one with the most information and most context.

I moved the line out, although now every time I initiate an LSP it causes an error. The repo doesn't have any more mentions of callbacks, so is this working right?

Maybe what we should do is check this each time a new config is registered (to make sure no deprecated settings are being added).

I'll look into that more tonight, since it looks like callbacks is getting populated still somehow.

tjdevries · 2020-11-14T22:25:01Z

lua/nvim_lsp/configs.lua

-  local function add_callbacks(config)
-    config.callbacks["window/logMessage"] = function(err, method, params, client_id)
+  local function add_handlers(config)
+    assert(not lsp.callbacks, "lsp.callbacks has been deprecated. See here for more: https://github.com/neovim/neovim/pull/12655")


Oh, we should also chdck that config.callbacks is nil.

Sorry to be so pedantic about this.

It's no problem! We want to make sure it's right before it goes someplace people use it everyday.

I'll add it in a little bit and push, just leaving this here to say I've seen it.

Cool, ping me on Monday if I haven't looked again. Idk what my schedule is tomorrow.

Iron-E · 2020-11-15T04:04:49Z

lua/nvim_lsp/configs.lua

@@ -34,7 +34,7 @@ function configs.__newindex(t, config_name, config_def)
  -- which is why this is a function, so that it can refer to the settings
  -- object on the server.
  local function add_handlers(config)
-    assert(not lsp.callbacks, "lsp.callbacks has been deprecated. See here for more: https://github.com/neovim/neovim/pull/12655")
+    assert(not config.callbacks, "lsp.callbacks has been deprecated. See here for more: https://github.com/neovim/neovim/pull/12655")


I tested JDT LS using the provided config on master and the proposed config in this pull. Using this assert it fails on master and works for patch-1 😌

As for lsp.callbacks, I tried the following:

nvim -u NORC -c 'lua assert(not vim.lsp.callbacks, "Failed!")'

and it failed. So I replaced the lsp reference with config.

ah, yeah, that's what I mean to say. I wanted you to assert about the config (which is the thing the user can control).

tjdevries · 2020-11-15T04:41:47Z

lua/nvim_lsp/configs.lua

-        assert(lsp.callbacks and lsp.callbacks[method], "Update to Nvim HEAD. This is an incompatible interface.")
-        assert(lsp.callbacks["window/showMessage"], "Callback for window/showMessage notification is not defined")
-        lsp.callbacks["window/showMessage"](err, method, params, client_id)
+        assert(lsp.handlers and lsp.handlers[method], "Update to Nvim HEAD. This is an incompatible interface.")


I don't think is needed anymore.

You're right. If we're aiming to assert on addition, we don't need to ship back a function which makes assertions about that again.

h-michael · 2020-11-15T06:24:10Z

@Iron-E Sorry I've merged this PR. Could you rebase?
#348

The recent changes in neovim head cause loading this file to create an error. These changes have resolved that on my machine

Iron-E · 2020-11-15T07:25:44Z

No problem. Good time to get that one through

lua/lspconfig/configs.lua

Co-authored-by: Hirokazu Hata <[email protected]>

h-michael

@tjdevries Is there anything else?

tjdevries · 2020-11-16T00:47:18Z

LGTM I think

h-michael · 2020-11-16T00:59:03Z

@Iron-E Thank you for your work :)

Iron-E · 2020-11-16T01:23:18Z

Happy we could do it!

Iron-E mentioned this pull request Nov 13, 2020

Use Handlers instead of Callbacks nvim-lua/completion-nvim#266

Closed

tjdevries requested changes Nov 14, 2020

View reviewed changes

tjdevries reviewed Nov 14, 2020

View reviewed changes

Iron-E commented Nov 15, 2020

View reviewed changes

tjdevries reviewed Nov 15, 2020

View reviewed changes

Iron-E and others added 7 commits November 15, 2020 02:21

Use handlers instead of callbacks

c78332d

The recent changes in neovim head cause loading this file to create an error. These changes have resolved that on my machine

Change the rest of the callback usages

584746a

Add help reference for failed callback assertion

046090c

Move no callbacks assertion to outer scope

26e787b

Put assertion within function scope

f1a7929

Use 'not config.callbacks' over 'not lsp.callbacks'

37c510c

Remove redundant assertion

e82159a

h-michael reviewed Nov 16, 2020

View reviewed changes

lua/lspconfig/configs.lua Outdated Show resolved Hide resolved

Iron-E and others added 2 commits November 15, 2020 19:33

Update lua/lspconfig/configs.lua

c070b9d

Co-authored-by: Hirokazu Hata <[email protected]>

Re-add negation after automated commit

f572120

h-michael approved these changes Nov 16, 2020

View reviewed changes

h-michael merged commit 35b0064 into neovim:master Nov 16, 2020

Iron-E deleted the patch-1 branch November 16, 2020 15:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use handlers instead of callbacks #399

Use handlers instead of callbacks #399

Iron-E commented Nov 13, 2020 •

edited by h-michael

Loading

lithammer commented Nov 13, 2020

Iron-E commented Nov 13, 2020

Iron-E commented Nov 13, 2020

teto commented Nov 14, 2020

tjdevries Nov 14, 2020

Iron-E Nov 14, 2020

tjdevries Nov 14, 2020

tjdevries Nov 14, 2020

Iron-E Nov 14, 2020

Iron-E Nov 14, 2020

tjdevries Nov 14, 2020

Iron-E Nov 15, 2020

tjdevries Nov 15, 2020

Iron-E Nov 15, 2020

tjdevries Nov 15, 2020

tjdevries Nov 15, 2020

Iron-E Nov 15, 2020

h-michael commented Nov 15, 2020

Iron-E commented Nov 15, 2020

h-michael left a comment

tjdevries commented Nov 16, 2020

h-michael commented Nov 16, 2020

Iron-E commented Nov 16, 2020

Use handlers instead of callbacks #399

Use handlers instead of callbacks #399

Conversation

Iron-E commented Nov 13, 2020 • edited by h-michael Loading

lithammer commented Nov 13, 2020

Iron-E commented Nov 13, 2020

Iron-E commented Nov 13, 2020

teto commented Nov 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h-michael commented Nov 15, 2020

Iron-E commented Nov 15, 2020

h-michael left a comment

Choose a reason for hiding this comment

tjdevries commented Nov 16, 2020

h-michael commented Nov 16, 2020

Iron-E commented Nov 16, 2020

Iron-E commented Nov 13, 2020 •

edited by h-michael

Loading