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

refactor: rebuild lsp command by api #2051

Closed
wants to merge 11 commits into from
Closed

refactor: rebuild lsp command by api #2051

wants to merge 11 commits into from

Conversation

glepnir
Copy link
Member

@glepnir glepnir commented Aug 5, 2022

cc @justinmk

Fix #2068

glepnir added 2 commits August 5, 2022 17:51
Signed-off-by: Raphael <glephunter@gmail.com>
@ranjithshegde
Copy link
Contributor

@glepnir #1984 already includes some of these changes

@glepnir glepnir marked this pull request as draft August 20, 2022 13:12
Comment on lines 10 to 13
if vim.fn.exists 'g:lspconfig' == 1 then
return
end
vim.g.lspconfig = 1
Copy link
Member

Choose a reason for hiding this comment

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

Can you move it to the top? So defining g:lspconfig will disable it entirely.

@@ -0,0 +1,58 @@
local api = vim.api
local lspconfig = require 'lspconfig'
Copy link
Member

Choose a reason for hiding this comment

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

Can require 'lspconfig' be inlined in all places where lspconfig is used? Then we don't force the module to be loaded.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Comment on lines 36 to 40
local lspconfig = require 'lspconfig'
local list = lspconfig.available_servers()
return vim.tbl_filter(function(s)
return string.match(s, '^' .. arg)
end, list)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
local lspconfig = require 'lspconfig'
local list = lspconfig.available_servers()
return vim.tbl_filter(function(s)
return string.match(s, '^' .. arg)
end, list)
return vim.tbl_filter(function(s)
return s:sub(1, #arg) == arg
end, require 'lspconfig'.available_servers())

Comment on lines 31 to 32
local lspconfig = require 'lspconfig'
lspconfig.lsp_start(args.args)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
local lspconfig = require 'lspconfig'
lspconfig.lsp_start(args.args)
require 'lspconfig'.lsp_start(args.args)

Copy link
Member

Choose a reason for hiding this comment

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

@glepnir you've missed this one

Copy link
Member Author

Choose a reason for hiding this comment

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

oh sry. fix

Comment on lines 46 to 49
local lspconfig = require 'lspconfig'
for _, client in ipairs(lspconfig.util.get_clients_from_cmd_args(cmd_args)) do
client.stop()
end
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
local lspconfig = require 'lspconfig'
for _, client in ipairs(lspconfig.util.get_clients_from_cmd_args(cmd_args)) do
client.stop()
end
for _, client in ipairs(require 'lspconfig'.util.get_clients_from_cmd_args(cmd_args)) do
client.stop()
end

Comment on lines 57 to 58
local lspconfig = require 'lspconfig'
lspconfig.lsp_restart(cmd_args)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
local lspconfig = require 'lspconfig'
lspconfig.lsp_restart(cmd_args)
require 'lspconfig'.lsp_restart(cmd_args)

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@justinmk
Copy link
Member

let's revisit after #1984 ? Though maybe this isn't needed if nvim_create_user_command serves the purpose well enough?

What's the use case that isn't covered by nvim_create_user_command ?

@glepnir
Copy link
Member Author

glepnir commented Aug 21, 2022

This is different from #1984. I just replaced lspconfig.vim. and fixed #2068 now we can use LspInfo border=single to set the border . why not provide a config to set the border. because I think the lspconfig just provide the server config. compare with vim.cmd use string format to generate the autocmd use api will be more better:) and neovim use lua file in everywhere now :)

@glepnir glepnir marked this pull request as ready for review August 21, 2022 22:01
Copy link
Member

@justinmk justinmk left a comment

Choose a reason for hiding this comment

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

yeah, this looks like good changes.

@ranjithshegde
Copy link
Contributor

ranjithshegde commented Aug 22, 2022

This conflicts with #1984 because lspconfig.lua logic introduced in that PR is incompatible with this PR.
All these user_commands are moved to plugin/lspconfig.lua, with lua/lspconfig.lua just creating/assigning a metatable for lsp_configurations.

I would recommend please wait till #1984 is merged. After that, this PR will simply be modifying the border key and make it accessible by fargs/nargs

@glepnir
Copy link
Member Author

glepnir commented Aug 22, 2022

@ranjithshegde because you don't reply my review so I continue work on this . maybe this will be merge or not @ii14 what do you think?

@ranjithshegde
Copy link
Contributor

@ranjithshegde because you don't reply my review so I continue work on this . maybe this will be merge or not @ii14 what do you think?

My apologies. For whatever reason I wasn't subscribed to this thread.

If you could please wait until that is merged, then the job of this PR will be very very simple. All the logic will already have been introduced in #1984,
then you will simply have to make the border key modifiable by user.

@glepnir
Copy link
Member Author

glepnir commented Aug 22, 2022

now we can use LspInfo border=single to set the border . why not provide a config to set the border. because I think the lspconfig just provide the server config.

this project only provide the server config. not provide any other config should be better. so pass the argument into the cmdline is my way. not use a border key config .

My apologies. For whatever reason I wasn't subscribed to this thread.

I had check your pr when you comment on this pr and I suggest you some changes like this pr. but you didn't reply .

@ii14
Copy link
Member

ii14 commented Aug 22, 2022

what do you think?

I don't know, I'm only reviewing the code :)

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.

Add a setup option to configure border of the floating window of LspInfo and others
4 participants