neovim icon indicating copy to clipboard operation
neovim copied to clipboard

feat(lsp): notify on number of changes for `vim.lsp.buf.rename`

Open chrisgrieser opened this issue 1 year ago • 4 comments

This is a small PR modifying vim.lsp.buf.rename(). It adds a notification on how many occurrences in how many files have changed, like inc-rename does.

My first PR to neovim core, so please feel free to correct me if I missed anything.

chrisgrieser avatar Dec 17 '23 11:12 chrisgrieser

I wonder if this will make too much unnecessary notification (through vim.notify, and there seems no possible way to turn it off. Isn't customization by overriding the LSP handler enough (in user config, or a plugin provides the functionality)?

vim.lsp.handlers[ms.textDocument_rename] = (function(original_handler)
  return function(err, result, ctx, config)
    -- TODO: Add Notification here
    original_handler(err, result, ctx, config)
  end
end)(vim.lsp.handlers[ms.textDocument_rename])

An alternative way one can consider is to make use of config table in the textDocument/rename handler, and allow easier customization via vim.lsp.with. But adding configuration points should be approached carefully.

wookayin avatar Dec 17 '23 17:12 wookayin

I wonder if this will make too much unnecessary notification

Imo, the fact that vim.lsp.buf.rename can apply changes to files that you haven't even opened is consequential enough to warrant a notification. Right now, it is too easy to accidentally make a change to several files without noticing it.

and there seems no possible way to turn it off

True. I wanted to be consistent with existing functions, and to my knowledge, none of them offer configurability regarding the display of notifications. But adding an option to opt-out (or even opt-in) of the notification shouldn't be an issue, if that's what everyone else thinks.

Isn't customization by overriding the LSP handler enough

In my view, overriding the LSP handler is not user-friendly or discoverable enough. Reading :h vim.lsp.buf.rename, for instance, there is no indication that it is possible to further customize the behavior via handler. And even with the information that handlers allow for more customization, I'd have a hard time as a new user to figure out how to do that after only reading :h lsp-handler.

chrisgrieser avatar Dec 18 '23 12:12 chrisgrieser

I wonder if this will make too much unnecessary notification

Imo, the fact that vim.lsp.buf.rename can apply changes to files that you haven't even opened is consequential enough to warrant a notification. Right now, it is too easy to accidentally make a change to several files without noticing it.

and there seems no possible way to turn it off

True. I wanted to be consistent with existing functions, and to my knowledge, none of them offer configurability regarding the display of notifications. But adding an option to opt-out (or even opt-in) of the notification shouldn't be an issue, if that's what everyone else thinks.

Isn't customization by overriding the LSP handler enough

In my view, overriding the LSP handler is not user-friendly or discoverable enough. Reading :h vim.lsp.buf.rename, for instance, there is no indication that it is possible to further customize the behavior via handler. And even with the information that handlers allow for more customization, I'd have a hard time as a new user to figure out how to do that after only reading :h lsp-handler.

I've got my hand-rolled handler that populates QF, and pops up a notification with multiple info points. Would be great not to have to add another override into noice to route these messages out of notifications.

pavkam avatar Dec 21 '23 20:12 pavkam

Okay then, I added an option for whether to show the notification.

I think you can make the argument to make it opt-out, since the notification is rather making the function more beginner-friendly, but to be as conservative as possible, I made it opt-in for now.


edit: On the failing tests

  • news failing is fine, since this change is too small for news.
  • The FreeBSD test failing I don't really understand, tbh.
  • docs are failing I can see, but running the recommended make doc gives the following error and I do not know what this is about?
cd .deps && \
                cmake -G 'Unix Makefiles'   \
                 /Users/chrisgrieser/repos/neovim//cmake.deps
/bin/sh: line 0: cd: .deps: No such file or directory
make: *** [build/.ran-deps-cmake] Error 1

chrisgrieser avatar Dec 22 '23 20:12 chrisgrieser