Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat!: 0.7 API update #1838

Closed
wants to merge 1 commit into from
Closed

feat!: 0.7 API update #1838

wants to merge 1 commit into from

Conversation

mjlbach
Copy link
Contributor

@mjlbach mjlbach commented Apr 15, 2022

  • switch to lua api for autocommands

  • switch to nvim_create_user_command

  • move to lua plugin initialization

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

    This will require users to update commands -> user_commands in the
    setup {} call.

@mjlbach mjlbach force-pushed the feat/0_7_goodies branch 13 times, most recently from d0e2e71 to abbeaf9 Compare April 15, 2022 22:58
@mjlbach mjlbach marked this pull request as ready for review April 15, 2022 22:58
@mjlbach mjlbach changed the title feat: switch to lua autocommands feat!: 0.7 API update Apr 15, 2022
Copy link

@zappolowski zappolowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, you've missed some spots which still reference the old .commands field. Using these changes, I at least got the basic functionality with user defined commands to work (default commands were working OOTB).

I'm still trying to understand how e.g. ranges are passed in Lua.

lua/lspconfig/configs.lua Outdated Show resolved Hide resolved
lua/lspconfig/configs.lua Outdated Show resolved Hide resolved
lua/lspconfig/configs.lua Outdated Show resolved Hide resolved
@mjlbach
Copy link
Contributor Author

mjlbach commented Apr 16, 2022

I'm still trying to understand how e.g. ranges are passed in Lua.

The lua callback receives a table as a single arg, it should be args.range

@zappolowski
Copy link

zappolowski commented Apr 16, 2022

I'm still trying to understand how e.g. ranges are passed in Lua.

The lua callback receives a table as a single arg, it should be args.range

Thanks, I've figured out the issue I've had before: opts for nvim_create_user_command cannot be defined by the user, but needs to be { range = true } then.

diff --git a/lua/lspconfig/configs.lua b/lua/lspconfig/configs.lua
index 245e32f..867754e 100644
--- a/lua/lspconfig/configs.lua
+++ b/lua/lspconfig/configs.lua
@@ -277,7 +277,7 @@ function configs.__newindex(t, config_name, config_def)
       M.user_commands = vim.tbl_deep_extend('force', M.user_commands, client.config.user_commands)
     end
     for _, command_info in pairs(M.user_commands or {}) do
-      vim.api.nvim_create_user_command(command_info.name, command_info.command, { desc = command_info.description })
+      vim.api.nvim_create_user_command(command_info.name, command_info.command, vim.tbl_extend('keep', command_info.opts or {}, { desc = command_info.description }))
       M.commands_created = true
     end
   end

@mjlbach mjlbach force-pushed the feat/0_7_goodies branch 4 times, most recently from 9d75fa6 to 7fc519f Compare April 16, 2022 16:12
@mjlbach
Copy link
Contributor Author

mjlbach commented Apr 16, 2022

Ok, I think we should be good now? If you want tot take another look. I made the defined user_commands match the exact function signature of create_user_command

Copy link

@zappolowski zappolowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just these two typos ... besides that, it works (for my use case) 👍

lua/lspconfig/configs.lua Outdated Show resolved Hide resolved
@mjlbach
Copy link
Contributor Author

mjlbach commented Apr 16, 2022

Whoops, bad force push, try now.

@zappolowski
Copy link

Thanks for your efforts. 👍

@mjlbach
Copy link
Contributor Author

mjlbach commented Apr 16, 2022

Thanks for catching my typos!

I'm going to give this a week to stew/people to read my warning message about breaking 0.6 compatibility before merging. Do you prefer the new interface for user_commands? I think it's more intuitive since the list maps directly onto the arguments passed to nvim_create_user_command now. Alternatively I could lift the name so that the table of user_commands is keyed by the command name instead of being a list, but IMO having the table include the exact arguments is cleaner. WDYT?

@zappolowski
Copy link

zappolowski commented Apr 17, 2022

I also prefer to have the API resemble nvim_create_user_command as close as possible (less cognitive load in total).

While playing around with both approaches I ran into an issue. Using the list like form of tables means, that vim.tbl_deep_extend as used here cannot be used anymore (it's intended for map-like tables). The attempted merge results in vanishing default defined commands when user defined commands are used as well.

@mjlbach
Copy link
Contributor Author

mjlbach commented Apr 17, 2022

That's a good point, we can either key the table with the name of the command as before, or merge the lists manually without extending the original table

@zappolowski
Copy link

Personally, I would stick to the new API with lists (as written before) and thus just merge the lists.

One thing (and that's actually how I found the issue) to consider then, is how to handle both intentional (e.g. when one wants to alter an existing command without having to come up with a new name) and unintentional (implementing a command which is then later provided by this plugin) overwriting of existing commands.

My proposal would be to use the default defined commands as the base list and add user defined commands in the end. This way both sets of commands should still work and intentional overwriting will work as well. I would guess that unintentional overwriting is rare enough to be dealt later.

@mjlbach
Copy link
Contributor Author

mjlbach commented Apr 17, 2022

We might even just do away with the user_command entrypoint to setup{}... there's no reason that a user can't just use on_init or on_attach to map the command themselves now that we have the lua api, before there was a motivation for the sugar as it took care of the annoyance of the vimscript api.

@mjlbach mjlbach force-pushed the feat/0_7_goodies branch 3 times, most recently from fd68d6c to f8ea151 Compare April 17, 2022 16:29
@mjlbach
Copy link
Contributor Author

mjlbach commented Apr 17, 2022

I decided to just have user_commands overwrite then trying to merge so users can disable the default command mappings, also I think if people really want to map their own commands they can just call the create_user_commands api directly. Still need to fix docgen and then this is g2g

ajitid added a commit to ajitid/dotfiles-2023 that referenced this pull request Apr 21, 2022
apply this branch after this PR is merged
neovim/nvim-lspconfig#1838
scheatkode added a commit to scheatkode/dotfiles that referenced this pull request Apr 23, 2022
Remove client.resolved_capabilities following neovim/nvim-lspconfig#1838

Also conditionally setting the mappings is not that helpful since it
silences the message saying such language server does not support such
capability.
* switch to lua api for autocommands
* switch to nvim_create_user_command
* move to lua plugin initialization
* rename commands -> user commands to match API and free up commands
  namespace

  This will require users to update commands -> user_commands in the
  setup {} call.

BREAKING CHANGE
ajitid added a commit to ajitid/dotfiles-2023 that referenced this pull request Apr 26, 2022
apply this branch after this PR is merged
neovim/nvim-lspconfig#1838
ajitid added a commit to ajitid/dotfiles-2023 that referenced this pull request Apr 29, 2022
apply this branch after this PR is merged
https://github .com/neovim/nvim-lspconfig/pull/1838
@ranjithshegde
Copy link
Contributor

Is anyone working on this since the departure of our beloved maintainer mjlbach?
I have been riding this branch since its creation and its been working great!
Willing to help out with tests or any other way necessary

@mjlbach
Copy link
Contributor Author

mjlbach commented May 8, 2022

I'm still alive fwiw :) I just wanted to set clear expectations on my response time/responsibilities (indefinite, and none) while I am on break.

The main reason I didn't merge this yet is because I was giving time for 0.7 to become the new standard. I know there is going to be some blowback dropping pre 0.7 support, but I wanted to do these cleanups for awhile.

@justinmk
Copy link
Member

justinmk commented Jul 4, 2022

#1984

@justinmk justinmk closed this Jul 4, 2022
@justinmk justinmk deleted the feat/0_7_goodies branch July 4, 2022 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants