Merge 0.7 branch (rebased to master) #1984

ranjithshegde · 2022-07-04T20:22:54Z

Since the 0.7 branch had no one taking initiative, this is essentially #1838 rebased to master.

I have been running the branch of the PR since it was created, have zero issues.
Seeing as 0.7.2 is released and master already requires nvim >=0.7, it seems like the ideal time to revisit and merge this.

All credits go to the excellent work of mjlbach, who I wont tag to respect his privacy.

justinmk · 2022-07-04T20:35:13Z

I have been running the branch of the PR since it was created, have zero issues.

Thank you! That is valuable to know.

Why again was commands renamed to user_commands ? Doesn't look like there was a technical reason for it, nor is it an aesthetic improvement. It implies a distinction between "non user commands", which doesn't serve any purpose. It's just an interface to allow specifying commands, there is no risk of confusion, so making users type out user_commands instead of a simpler name is just pedantry.

ranjithshegde · 2022-07-04T20:39:06Z

I have been running the branch of the PR since it was created, have zero issues.

Thank you! That is valuable to know.

Why again was commands renamed to user_commands ?

This is straight from the comments on the main PR

"rename commands -> user commands to match API and free up commands
namespace"

On the one hand, the api resemblance is nice. But do you think its too confusing for the users? as these commands are essentially wrappers around unique lsp/commands?

Edit: after reading your edits, do you say it just be changed back to commands?

justinmk · 2022-07-04T20:41:43Z

"API resemblance" here means "it literally matches nvim_create_user_command ? This is questionable because:

nvim_create_user_command itself is probably over-specific and should have been named nvim_create_command (if ever it becomes theoretically possible to define non-user commands, why would we want a separate interface?! and, if it never becomes possible, what's the point of mentioning it over and over? Not even :command itself mentions this, it's literaly named :command.)
this is not a strong enough motivation to ask users to perform a migration

ranjithshegde · 2022-07-04T20:44:32Z

"API resemblance" here means "it literally matches nvim_create_user_command ? This is questionable because:

nvim_create_user_command itself is probably over-specific and should have been named nvim_create_command (if ever it becomes theoretically possible to define non-user commands, why would we want a separate interface?! and, if it never becomes possible, what's the point of mentioning it over and over? Not even :command itself mentions this, it's literaly named :command.)

this is not a strong enough motivation to ask users to perform a migration

Will change it back to command

justinmk · 2022-07-04T20:55:03Z

Looking closer, and reviewing #1838 , I see that a migration will be required anyway, because the command structure changed.

I wonder if instead we should maintain the old interface. And deprecate it. Because it's not really needed, people can just call nvim_create_user_comand from on_init or on_attach.

ranjithshegde · 2022-07-04T21:04:11Z

Looking closer, and reviewing #1838 , I see that a migration will be required anyway, because the command structure changed.

I wonder if instead we should maintain the old interface. And deprecate it. Because it's not really needed, people can just call nvim_create_user_comand from on_init or on_attach.

Perhaps we remove the interface for user to define the commands, seeing as they could just use nvim_create_user_command where appropriate. But many internally defined commands (ClangdSwitchSourceHeader as an example) rely on this mechanism.

Not sure if any language specific plugins make use of them either.

So perhaps just remove it from user_space?

EDIT: Defining the commands in the setup/config of the language_server has the advantage of being able to access the server specific functions with little code.

An example from the help.txt of this PR

  commands = {
     {
      name = "TexlabBuild"
      command = function()
        buf_build(0)
      end,
      opts = {
        desc = "Build the current buffer",
      }
    },
  },

---The `configs.__newindex` metamethod consumes the config definition and returns
---an object with a `setup()` method, to be invoked by users:

    require'lspconfig'.SERVER_NAME.setup{}

---After you set `configs.SERVER_NAME` you can add arbitrary language-specific
---functions to it if necessary.

---Example
    configs.texlab.buf_build = buf_build

justinmk · 2022-07-05T11:18:36Z

Perhaps we remove the interface for user to define the commands, seeing as they could just use nvim_create_user_command where appropriate.

I'm in favor of that. We would need to provide a simple example of using nvim_create_user_command in on_attach.

EDIT: Defining the commands in the setup/config of the language_server has the advantage of being able to access the server specific functions with little code.

An example from the help.txt of this PR

I don't see the advantage. The nvim_create_user_command call could just reference require'lspconfig'.texlab.buf_build.

ranjithshegde · 2022-07-05T11:48:43Z

I don't see the advantage. The nvim_create_user_command call could just reference require'lspconfig'.texlab.buf_build.

Many methods from this repo (chiefly lspconfig/utils like vim.fs) are already in core, along with the addition of au-events like LspAttach.
Maybe after the release of nvim 0.8 + an appropriate "settle-in" period, this repo could be stripped down to a growing collection of lsp-configs with strong defaults?

With that in mind, this PR could supply a deprecation warning concerning commands, with some examples of how to set the nvim_create_user_commands in an on_attach function?
Removing it now might be too radical for some users just yet!

ranjithshegde · 2022-07-24T15:59:35Z

I have added a deprecation notice for commands as discussed. There is also an example in lspconfig.txt to set them up in on_attach function.

Still not entirely sure this is ideal, having commands in individual LSP setups seems cleaner than having them in a common on_attach function where the commands are filtered by client.name.

@justinmk Would appreciate your review and thoughts on this matter

ranjithshegde · 2022-08-06T16:59:45Z

@justinmk Gentle ping
Requesting your review

ranjithshegde · 2022-08-19T08:10:16Z

Not sure who the maintainers are now to request review from.
@williamboman?

doc/lspconfig.txt

lua/lspconfig/configs.lua

lua/lspconfig/server_configurations/zk.lua

plugin/lspconfig.lua

test/minimal_init.lua

williamboman

I'm personally really opposed to breaking the command API, if the plan is to deprecate it anyway.

lua/lspconfig/server_configurations/zk.lua

lua/lspconfig/configs.lua

plugin/lspconfig.lua

scripts/docgen.lua

lua/lspconfig/configs.lua

plugin/lspconfig.lua

doc/lspconfig.txt

lua/lspconfig/configs.lua

justinmk · 2022-08-21T21:36:14Z

When I opened this PR (still 95% his work), other maintainers first suggested it be just called commands instead, which I changed, then it was discussed it was best to deprecate it entirely.

Right now it reflect all of those changes, in a single PR!. What is the suggestion? Keep it as the original PR by mjlbach and then deprecate it in the next nvim release cycle? Or something else?

@ranjithshegde this is my fault, sorry. Let's see if we can make progress here without too much work. Here's what I suggest:

soft deprecate (show warning/error but continue to work) the commands API without a breaking change (so don't need to change the code)
add the documentation examples for nvim_create_user_command
remove the examples for commands
update the existing lspconfigs to avoid commands

ranjithshegde · 2022-08-23T08:06:49Z

Is there a way to see on which file and line codespell action fails at?

glepnir · 2022-08-23T08:08:08Z

* test: add failing test * fix(lspconfig): only consider servers that have been set up as available

plugin/lspconfig.lua

ranjithshegde · 2022-08-23T13:31:57Z

test/lspconfig_spec.lua

-        return lspconfig.available_servers()
+        return require("lspconfig.util").available_servers()


Well at the moment, the lspconfig.lua simply sets up the metatable for lspconfig.configs and nothing else. If we add this, then it will setup a metatable + a single function that can actually be in utils.
Utils holds all similar fucntions such as get_managed_clients, get_active_clients. It is perhaps a better place for it.

Then the logic is simple. The "main" portion of plugin simply sets up LSPs. Some commands in plugin/ and some useful functions in /utils.

Any thoughts?

Also, too many PRs are being merged that potentially clash with this. Requires a bit of reshuffling

Then the logic is simple. The portion of plugin simply sets up LSPs, with some commands in plugin/ and some useful functions in /utils.

Yeah I agree it makes sense to remove it, I'm just thinking about not breaking parity with master. This will for sure break some user configs and plugins out there, although easily remedied.

Also, too many PRs are being merged that potentially clash with this. Requires a bit of reshuffling

Yeah there seems to be a lot happening with this big PR still being open, I'll eject myself from further comments, let's get this merged haha

Yeah I agree it makes sense to remove it, I'm just thinking about not breaking parity with master. This will for sure break some user configs and plugins out there, although easily remedied.

Well this is changing/simplifying the core mechanism of the project. We could merge this as a breaking change, with a tag for previous versions?

@williamboman Seems this discussion was not reflected in the commit messages. Whats the best stragety to notify that M.available_servers have been moved to utils?

justinmk

Thanks everyone for the help here.

* switch to lua api for autocommands * switch to nvim_create_user_command * move to lua plugin initialization NOTICE: Defining commands in server configurations will be deprecated in future releases. Please refer to the exaple in `:help lspconfig.txt` to setup the same in an `on_attach` function

ranjithshegde · 2022-08-23T14:00:52Z

Squashed all the changes into single commit!

justinmk · 2022-08-23T14:02:33Z

Will merge this now. Can someone open a PR that removes stuff for 0.8? It won't be merged until 0.8 is released, but a PR will help us figure out what changes are expected.

ranjithshegde · 2022-08-23T14:04:10Z

Will merge this now. Can someone open a PR that removes stuff for 0.8? It won't be merged until 0.8 is released, but a PR will help us figure out what changes are expected.

Will start with an issue. Maybe deprecation is requried as the first step. Many functions in util are available upstream.
A strategy could be to redirect those calls to upstream with a deprecation warning

Commit neovim#1984 included a breaking change. Function `require("lspconfig").available_servers` has been moved to the utils module. The new call is `require("lspconfig.util").available_servers`

BREAKING CHANGE: Functions from `lspconfig.lua` Commit neovim#1984 included a breaking change. Function `require("lspconfig").available_servers` has been moved to the utils module. The new call is `require("lspconfig.util").available_servers`

mike-lloyd03 · 2022-08-23T23:07:38Z

I almost just opened an issue for this but did a bit more digging first. I see that this commit introduces a breaking change. But I cannot tell from the conversation here what needs to be changed in my config to accommodate this change. Or if it's a problem with another plugin that depends on lspconfig and I should roll back to a previous version of this plug until other plug developers can update. Is there recommended guidance somewhere I should be looking at?

Thanks and sorry for this annoying message. I did my research first but came up short.

justinmk · 2022-08-23T23:18:31Z

@ranjithshegde I wasn't expecting breaking changes here per #1984 (comment) :

soft deprecate (show warning/error but continue to work) the commands API without a breaking change (so don't need to change the code)

What did we break?

ii14 · 2022-08-23T23:19:34Z

@justinmk looks like require("lspconfig").available_servers is now require("lspconfig.util").available_servers.

ii14 · 2022-08-23T23:23:05Z

If this is a problem, it could just be added back:

diff --git a/lua/lspconfig.lua b/lua/lspconfig.lua
index 1178202..7838f53 100644
--- a/lua/lspconfig.lua
+++ b/lua/lspconfig.lua
@@ -4,6 +4,11 @@ local M = {
   util = require 'lspconfig.util',
 }
 
+function M.available_servers()
+  vim.deprecate('lspconfig.available_servers', 'lspconfig.util.available_servers', '0.1.4', 'lspconfig')
+  return M.util.available_servers()
+end
+
 local mt = {}
 function mt:__index(k)
   if configs[k] == nil then

edit: added vim.deprecate

glepnir · 2022-08-23T23:35:23Z

@justinmk like i said in #2078 I had search in github some configs and plugins use it .but it move to utils.lua

justinmk mentioned this pull request Jul 4, 2022

feat!: 0.7 API update #1838

Closed

ranjithshegde force-pushed the 0.7 branch from db0c9dd to 8ba4dc6 Compare July 24, 2022 14:53

ranjithshegde force-pushed the 0.7 branch 5 times, most recently from 1b24cb4 to cf1c616 Compare August 2, 2022 16:18

ranjithshegde force-pushed the 0.7 branch 2 times, most recently from 5b3c2cc to ec8a6a0 Compare August 6, 2022 16:55

ranjithshegde mentioned this pull request Aug 9, 2022

refactor: rebuild lsp command by api #2051

Closed

kylo252 reviewed Aug 20, 2022

View reviewed changes

williamboman reviewed Aug 21, 2022

View reviewed changes

justinmk reviewed Aug 21, 2022

View reviewed changes

doc/lspconfig.txt Outdated Show resolved Hide resolved

justinmk reviewed Aug 21, 2022

View reviewed changes

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

ranjithshegde force-pushed the 0.7 branch 2 times, most recently from 5cb37e5 to 5851ac0 Compare August 22, 2022 15:00

ranjithshegde requested review from justinmk and removed request for justinmk August 22, 2022 15:33

ranjithshegde force-pushed the 0.7 branch from d96f45b to 1839586 Compare August 23, 2022 08:03

ranjithshegde force-pushed the 0.7 branch 4 times, most recently from ae467d4 to 61e2a79 Compare August 23, 2022 08:23

ranjithshegde requested review from kylo252 and removed request for justinmk August 23, 2022 08:25

glepnir mentioned this pull request Aug 23, 2022

feat: allow :LspStop and :LspRestart commands to apply to current buffer clients #1782

Closed

ranjithshegde referenced this pull request Aug 23, 2022

fix: only consider servers that have been set up as available (#2066)

6c3cbbc

* test: add failing test * fix(lspconfig): only consider servers that have been set up as available

ranjithshegde force-pushed the 0.7 branch from 61e2a79 to cb5a58d Compare August 23, 2022 12:56

justinmk reviewed Aug 23, 2022

View reviewed changes

plugin/lspconfig.lua Show resolved Hide resolved

williamboman reviewed Aug 23, 2022

View reviewed changes

justinmk approved these changes Aug 23, 2022

View reviewed changes

ranjithshegde force-pushed the 0.7 branch from 63f539f to 4b38f0a Compare August 23, 2022 14:00

justinmk merged commit fe7a6f4 into neovim:master Aug 23, 2022

ranjithshegde mentioned this pull request Aug 23, 2022

chore(notice): Breaking change notice #2078

Closed

ranjithshegde mentioned this pull request Aug 26, 2022

fix: remove the config.commands #2092

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge 0.7 branch (rebased to master) #1984

Merge 0.7 branch (rebased to master) #1984

ranjithshegde commented Jul 4, 2022

justinmk commented Jul 4, 2022 •

edited

Loading

ranjithshegde commented Jul 4, 2022 •

edited

Loading

justinmk commented Jul 4, 2022 •

edited

Loading

ranjithshegde commented Jul 4, 2022

justinmk commented Jul 4, 2022 •

edited

Loading

ranjithshegde commented Jul 4, 2022 •

edited

Loading

justinmk commented Jul 5, 2022 •

edited

Loading

ranjithshegde commented Jul 5, 2022 •

edited

Loading

ranjithshegde commented Jul 24, 2022

ranjithshegde commented Aug 6, 2022

ranjithshegde commented Aug 19, 2022

williamboman left a comment

justinmk commented Aug 21, 2022 •

edited

Loading

ranjithshegde commented Aug 23, 2022

glepnir commented Aug 23, 2022

This comment was marked as off-topic.

ranjithshegde Aug 23, 2022 •

edited

Loading

ranjithshegde Aug 23, 2022

williamboman Aug 23, 2022

ranjithshegde Aug 23, 2022

ranjithshegde Aug 23, 2022

justinmk left a comment

ranjithshegde commented Aug 23, 2022

justinmk commented Aug 23, 2022 •

edited

Loading

ranjithshegde commented Aug 23, 2022

mike-lloyd03 commented Aug 23, 2022

justinmk commented Aug 23, 2022

ii14 commented Aug 23, 2022

ii14 commented Aug 23, 2022 •

edited

Loading

glepnir commented Aug 23, 2022

		return lspconfig.available_servers()
		return require("lspconfig.util").available_servers()

Merge 0.7 branch (rebased to master) #1984

Merge 0.7 branch (rebased to master) #1984

Conversation

ranjithshegde commented Jul 4, 2022

justinmk commented Jul 4, 2022 • edited Loading

ranjithshegde commented Jul 4, 2022 • edited Loading

justinmk commented Jul 4, 2022 • edited Loading

ranjithshegde commented Jul 4, 2022

justinmk commented Jul 4, 2022 • edited Loading

ranjithshegde commented Jul 4, 2022 • edited Loading

justinmk commented Jul 5, 2022 • edited Loading

ranjithshegde commented Jul 5, 2022 • edited Loading

ranjithshegde commented Jul 24, 2022

ranjithshegde commented Aug 6, 2022

ranjithshegde commented Aug 19, 2022

williamboman left a comment

Choose a reason for hiding this comment

justinmk commented Aug 21, 2022 • edited Loading

ranjithshegde commented Aug 23, 2022

glepnir commented Aug 23, 2022

This comment was marked as off-topic.

ranjithshegde Aug 23, 2022 • edited Loading

Choose a reason for hiding this comment

ranjithshegde Aug 23, 2022

Choose a reason for hiding this comment

williamboman Aug 23, 2022

Choose a reason for hiding this comment

ranjithshegde Aug 23, 2022

Choose a reason for hiding this comment

ranjithshegde Aug 23, 2022

Choose a reason for hiding this comment

justinmk left a comment

Choose a reason for hiding this comment

ranjithshegde commented Aug 23, 2022

justinmk commented Aug 23, 2022 • edited Loading

ranjithshegde commented Aug 23, 2022

mike-lloyd03 commented Aug 23, 2022

justinmk commented Aug 23, 2022

ii14 commented Aug 23, 2022

ii14 commented Aug 23, 2022 • edited Loading

glepnir commented Aug 23, 2022

justinmk commented Jul 4, 2022 •

edited

Loading

ranjithshegde commented Jul 4, 2022 •

edited

Loading

justinmk commented Jul 4, 2022 •

edited

Loading

justinmk commented Jul 4, 2022 •

edited

Loading

ranjithshegde commented Jul 4, 2022 •

edited

Loading

justinmk commented Jul 5, 2022 •

edited

Loading

ranjithshegde commented Jul 5, 2022 •

edited

Loading

justinmk commented Aug 21, 2022 •

edited

Loading

ranjithshegde Aug 23, 2022 •

edited

Loading

justinmk commented Aug 23, 2022 •

edited

Loading

ii14 commented Aug 23, 2022 •

edited

Loading