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

docs: fix for Neovim LSP/Mason #122

Merged
merged 1 commit into from
Sep 20, 2024
Merged

Conversation

lcheylus
Copy link
Contributor

@lcheylus lcheylus commented Sep 19, 2024

  • add URL for mason registry
  • add URL for mason-lspconfig.nvim plugin (':LspInstall' command)
  • add URL for mason.nvim plugin (':MasonInstall v_analyzer' command)

@ttytm
Copy link
Member

ttytm commented Sep 19, 2024

Thanks for pointing that out. Last time checked, it was installable with lspconfig. If it isn't anymore, a fix should be submitted upstream imho. The LspInstall command is more commonly used for language servers and MasonInstall works then inherently

@lcheylus
Copy link
Contributor Author

lcheylus commented Sep 19, 2024

There is no LspInstall command in lspconfig Neovim plugin, see docs => https://github.com/neovim/nvim-lspconfig/blob/master/doc/lspconfig.txt "nvim-lspconfig does not install language servers for you"
The lspconfig Neovim plugin is only used for LSP servers configuration, Mason is used for installation. Personally, I always install manually my LSP servers (C, Go, Rust, Lua, Python) manually, not via Mason.

I don't know how you were able to run it ?!?!

@spytheman
Copy link
Member

spytheman commented Sep 19, 2024

Can we keep both?

note: personally I do not use neovim, so it may not make sense at all, in which case, tell me to shut up.

@lcheylus
Copy link
Contributor Author

Can we keep both?

No, reference to :LspInstall must be removed in README: this command does not exist in Neovim lspconfig plugin.

@lcheylus lcheylus force-pushed the docs-lspconfig branch 2 times, most recently from 14ab9a0 to 8d063c4 Compare September 19, 2024 13:41
@ttytm
Copy link
Member

ttytm commented Sep 19, 2024

Yep it's true what you are saying for default lspconfig. I'm referring to mason-lspconfig

@ttytm
Copy link
Member

ttytm commented Sep 19, 2024

https://github.com/williamboman/mason.nvim?tab=readme-ov-file#how-to-use-installed-packages also lists mason-lspconfig to use it for lsp installation, so i'd argue it's more ideomatic in the eco system.

@ttytm
Copy link
Member

ttytm commented Sep 19, 2024

Can we keep both?

note: personally I do not use neovim, so it may not make sense at all, in which case, tell me to shut up.

Weighing up the options and beeing over pedantic:
Keeping it both is no harm and would prevent such misconceptions. Tho if users have missed to install mason-lspconfig and only use mason.nvim they are missing out on some features that's are intended to work together. So forcing them towards there luck might eventually be better for the users. If someone knows about it and is deliberately not using it MasonInstall still works for those who know what they are doing

Edit: please don't mind the spelling issues, I'm having accessibility issues on my phone here 😅

@lcheylus
Copy link
Contributor Author

Weighing up the options and beeing over pedantic:
Keeping it both is no harm and would prevent such misconceptions. Tho if users have missed to install mason-lspconfig and only use mason.nvim they are missing out on some features that's are intended to work together. So forcing them towards there luck might eventually be better for the users. If someone knows about it and is deliberately not using it MasonInstall still works for those who know what they are doing

OK, I finally understood the error in README for Neovim configuration: :LspInstall is a command to install v-analyzer server with mason-lspconfig.nvim plugin, not lspconfig plugin (2 differents Neovim plugins: one for LSP configuration, the other for installation).

I will modify my PR to keep the both methods for installation:

  • :LspInstall with mason-lspconfig.nvim plugin
  • :MasonInstall with mason.nvim plugin

- add URL for mason registry
- add URL for mason-lspconfig.nvim plugin (':LspInstall' command)
- add URL for mason.nvim plugin (':MasonInstall v_analyzer' command)

Signed-off-by: Laurent Cheylus <[email protected]>
@lcheylus
Copy link
Contributor Author

Fix my PR:

  • add URL for mason registry
  • add URL for mason-lspconfig.nvim plugin (':LspInstall' command)
  • add URL for mason.nvim plugin (':MasonInstall v_analyzer' command)

@ttytm
Copy link
Member

ttytm commented Sep 20, 2024

I just see we already have both commands (I only addressed the commonts on my phone yesterday without checking the diff). In my memory it was just LspInstall without mentioning nvim-lspconfig. My comment regarding weighing up not to add MasonInstall makes less sense than.

Clarifying the current commands is fine 👍

@spytheman
Copy link
Member

Is it good to merge as it is now?

@lcheylus
Copy link
Contributor Author

Is it good to merge as it is now?

Yes :)

@spytheman spytheman merged commit 4f93826 into vlang:main Sep 20, 2024
@lcheylus lcheylus deleted the docs-lspconfig branch September 20, 2024 11:40
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.

3 participants