williamboman / nvim-lsp-installer

Further development has moved to https://github.com/williamboman/mason.nvim!
https://github.com/williamboman/mason.nvim
Apache License 2.0
2k stars 123 forks source link

[RFC] More discoverable identifiers for the :LspInstall, :LspUninstall commands #177

Closed williamboman closed 2 years ago

williamboman commented 2 years ago

Background

The :LspInstall and :LspUninstall commands currently only allow providing the lspconfig server names. This can be somewhat confusing for people who have not spent any time on LSP development - for example a Python LSP can be installed via :LspInstall pyright, :LspInstall pylsp, and :LspInstall jedi_language_server. Unless you are familiar with these Python LSPs it's 1) difficult to discover these in the first place (you may get lucky by typing py<Tab>), and 2) knowing which one to choose.

Proposal

1. Prefix server names with a more discoverable name

Each server may be associated with one or more constructs that are more accessible to the average user. Arguably, the dominant construct in this context would be which language/framework/library the LSP targets. This mapping is already provided in the README.

These prefixes will only be available for autocompletion in the command line. Calls to the underlying Lua APIs would not respect the prefix, and will return error should a prefix be included. In order to reduce noise, the server's true names will no longer be available for autocompletion in the command line - but they will still be considered valid inputs.

Examples

Considerations to be made


2. Introduce a new :LspInstallLanguage command

A new :LspInstallLanguage command could allow us to have two neatly separated commands for installing a server by using a different identifier. We could also use this command to impose a recommended, default, server per language. In the case where there are more than one LSP per language, this recommended server could either be automatically chosen, or we could prompt the user which one to install (and have the prompt default to the recommended one).

Examples

With prompt:

:LspInstallLanguage python
Please choose which server you want to install.
(1) pyright (recommended), [2] pylsp, (3) jedi_language_server:

We could also use the prompt to signal servers that are unstable, for example if the server has a questionable implementation of the LSP spec. This could potentially help managing expectations and reducing volume of related support issues.

:LspInstallLanguage csharp
Please choose which server you want to install.
(1) omnisharp (unstable)

Resolve by

25th October 2021

williamboman commented 2 years ago

Ping @shaunsingh and perhaps @kylo252 for the LunarVim perspective. Maybe @mjlbach has some opinions?

kylo252 commented 2 years ago

At first glance, I think I'm leaning towards the second option more, but with minor changes just for the sake of avoiding ambiguity:

LspInstall -> LspInstallServer LspInstallLanguage -> LspInstallServerPerLanguage


There's also a third alternative:

LspInstall lua 
# installing sumneko_lua server

LspInstall python
# multiple options are available, select which server you want to install:
(1) pyright, (2) pylsp, (3) jedi_language_server:

LspInstall python:pylsp
# installing pylsp server

It's worth mentioning that most people use this API interactively, so I don't think you need to worry about any "deprecation" notices in this case. As long you're using the same prefix, then autocomplete will take care of it for you. That also means it might be a good chance to re-think some of these function names if you want.

abalmos commented 2 years ago

LspInstallLanguage -> LspInstallServerPerLanguage

Maybe LspInstallServerFor lua rust python ?

williamboman commented 2 years ago

I'm not too keen on removing :LspInstall for something else. Reason being that I believe it's a very established command by now, where it's commonly referred to either through word of mouth, tutorials, or docs. Somewhat related: I recently learned that lspconfig used to have an :LspInstallInfo command prior to removing server installation support. I can't remember where I saw it but I believe it was in some kind of doc. I just happened to pick the same name for the command in this plugin, but this also meant that the correctness of these old docs remained intact.


LspInstall lua 
# installing sumneko_lua server

LspInstall python
# multiple options are available, select which server you want to install:
(1) pyright, (2) pylsp, (3) jedi_language_server:

LspInstall python:pylsp
# installing pylsp server

Yeah this seems like a nice design. I've given it some more thought and I'd like to stick to :LspInstall as long as we can figure out something intuitive. The only drawback (which I'm not even sure it is) is that it'd be awkward to allow for specifying which version of the LSP server you want to install when providing a language identifier (:LspInstall python@1.0.0??). But it'd just be plain weird to even support it when providing a language identifier and not a discrete server identifier.

I also think tab completion could work pretty neatly here, like:

python
python:pylsp
python:pyright
python:jedi_language_server
:LspInstall py<TAB>
shaunsingh commented 2 years ago

Ping @shaunsingh

LGTM. If there are multiple options, maybe have a recommended default server? (e.g. pyright is pretty much accepted as the goto python server)

abalmos commented 2 years ago

I also agree that sticking with a simple, single LspInstall might be the best option.

Seems like mapping languages to language servers is more a UI problem. In other words, I can directly issue LspInstall <server-name> if I know the server I want, otherwise, I really just want to see a list of what is supported so I can start learning about them.

Maybe a way to organize the UI list by language? I also very much like the idea (from another issue) that this plugin could suggest language servers based on your current buffer file types.

kylo252 commented 2 years ago

@williamboman as long as we can figure out something intuitive.

Would you consider using Telescope for this? or do you want to keep it simpler/independent?

williamboman commented 2 years ago

Would you consider using Telescope for this? or do you want to keep it simpler/independent?

For sure an independent thing, sticking to first principles. If someone wants to hook it into Telescope that should be totally doable using this plugin's Lua APIs

isaif commented 2 years ago

There's also a third alternative:

LspInstall lua 
# installing sumneko_lua server

LspInstall python
# multiple options are available, select which server you want to install:
(1) pyright, (2) pylsp, (3) jedi_language_server:

LspInstall python:pylsp
# installing pylsp server

I like this more than other ideas but I will like to suggest something.

Instead of doing LspInstall python:pylsp lets stick with LspInstall pylsp for installing specific servers. The reason being the user already knows which server they want.

If user uses command Lspinstall python then provide all the alternative servers available but if the command is lspinstall pylsp just install that server.

williamboman commented 2 years ago

So after letting this brew for a while I think the best experience would be what @isaif suggested, with the :LspInstall command behaving like today. We'd simply just extend its completion with virtual entries ("aliases") that would map to one or more actual servers. This would for example look like this, using confirm() to prompt the user for a selection:

:LspInstall python
Multiple options are available, please select which server you want to install:
(1) pyright, (2) pylsp, (3) jedi_language_server

These aliases would only be available in the command-line as an autocompletion item. Calling the Lua APIs (e.g. .install_server("python")) would render errors.

williamboman commented 2 years ago

In terms of how to implement this. I'm figuring that there should be a new requirement that each nvim-lsp-installer server instance must provide a list of languages as a property. This will in most cases just be a one-item list with a single language, like languages = { "python" }. This property will be used by the metadata autogen script to create look-up tables for fast access and ease of governance.

By having each server provide a list of languages we need to avoid these languages from carrying over to the autocompletion list as it would only pollute it in most cases. I'm thinking we could apply some intuitive heuristics, for example excluding language servers that fulfill the following conditions:

1) The server name has a similar name as its language. The similarity criterion can be whatever, but I figure a simple language.indexOf(serverName) == 1 would suffice. The rationale here is that adding a virtual entry would not really help users discover it. 2) The associated languages are only related to this one server.

williamboman commented 2 years ago

The above spec produces the following. Seems somewhat decent?

-- THIS FILE IS GENERATED. DO NOT EDIT MANUALLY.
-- stylua: ignore start
return {
  ["c++"] = { "clangd" },
  csharp = { "omnisharp" },
  d = { "serve_d" },
  eslint = { "eslint", "eslintls" },
  fortran = { "fortls" },
  haskell = { "hls" },
  java = { "jdtls" },
  javascript = { "rome", "tsserver" },
  latex = { "ltex", "texlab" },
  lua = { "sumneko_lua" },
  php = { "intelephense", "phpactor" },
  python = { "jedi_language_server", "pylsp", "pyright" },
  ruby = { "solargraph" },
  sql = { "sqlls", "sqls" },
  terraform = { "terraformls", "tflint" },
  typescript = { "rome", "tsserver" },
  vue = { "volar", "vuels" },
  xml = { "lemminx" },
  zig = { "zls" }
}
williamboman commented 2 years ago

Opened draft PR in #232, give it a spin and tell me what you think

kylo252 commented 2 years ago

I'm starting to have second thoughts on the "language" sorting. It feels like drawing a line in the sand on what's considered a language, e.g. eslint, tailwind and terraform have all somehow made it to that list but not markdown or html.

I think that we should actually stick to "filetypes" as an alternate identifier, for both clarity and correctness. I'm also wondering why not just re-use this https://github.com/williamboman/nvim-lsp-installer/blob/4afdea2ad3bdd970b0e4fc442de4af41017730ae/lua/nvim-lsp-installer/_generated/filetype_map.lua

I think a simple heuristic to apply is to exclude adding any filetype that shares the same 3 letters with the name of its server.


Edit: Here's a PR that demonstrates this https://github.com/williamboman/nvim-lsp-installer/pull/233

williamboman commented 2 years ago

I'm starting to have second thoughts on the "language" sorting. It feels like drawing a line in the sand on what's considered a language, e.g. eslint, tailwind and terraform have all somehow made it to that list but not markdown or html.

Yeah I agree that what constitutes a language is a bit fuzzy perhaps. The way I approached it was to aim for something balanced that would lead to the highest number of "hit rates" in terms of users trying to find it. As for what ends up in that language mapping follows the logic described in https://github.com/williamboman/nvim-lsp-installer/issues/177#issuecomment-955815529, so in the case of html it does not show up because there's only one language for "HTML" and it's underlying LSP name happens to be html.

I don't think it's too important that we have a perfect mapping here, I think for these purposes something that is "good enough" will suffice (which we can iterate on over time).

I think that we should actually stick to "filetypes" as an alternate identifier, for both clarity and correctness. I'm also wondering why not just re-use this https://github.com/williamboman/nvim-lsp-installer/blob/4afdea2ad3bdd970b0e4fc442de4af41017730ae/lua/nvim-lsp-installer/_generated/filetype_map.lua

I was thinking about this too but decided to not explore it. Filetypes can often be very esoteric, and while one could argue it would lead to a more correct mapping, I think the resulting UX will be worse. I've attached two screenshots of examples when using filetypes leads to ambiguity and noise. Not to mention certain servers (couh tailwind) is registered to a huge amount of filetypes.

I think using a direct filetype mapping could be interesting to allow installing an LSP that is relevant to one's currently open buffer. This could for example be achieved by just entering :LspInstall.

Screenshot 2021-11-01 at 14 22 45 Screenshot 2021-11-01 at 14 22 38
kylo252 commented 2 years ago

I had forgotten about the case where the server is called, by lspconfig, the same as the filetype. I fixed that in my PR and here's the result.

The algorithm is still pretty simple:

    for ft, names in pairs(filetype_map) do
        if not has_matching_prefix(ft, names) and not vim.tbl_contains(names, ft) then
            table.insert(uncommon_names, ft)
        else
            table.insert(skipped_filetypes, ft)
        end
    end

and I would say the results should be what anyone would expect

https://user-images.githubusercontent.com/59826753/139705063-31e443ec-1c1a-44f7-a2d4-0b5e639768d7.mp4

:

williamboman commented 2 years ago

and I would say the results should be what anyone would expect

Hmm I'm still not convinced. Been playing around with that branch for a while and I have no idea what half the names even are, nor what underlying servers they're mapped to. I think tailwindcss is the culprit for many of these, but it's a general observation.

I think using filetypes as a method to discover LSPs is interesting, but perhaps not as a source for autocompletion. What about allowing :LspInstall without any arguments, in which case it'd use the current buffer's (or perhaps all open buffers and make it interactive) filetype. We already kind of do this in the UI, where relevant LSPs are hoisted in the UI and receive a blue list icon.

Screenshot 2021-11-02 at 13 48 54 Screenshot 2021-11-02 at 13 49 06 Screenshot 2021-11-02 at 13 49 23 Screenshot 2021-11-02 at 13 51 32 Screenshot 2021-11-02 at 13 56 57

williamboman commented 2 years ago

What do you others think?

kylo252 commented 2 years ago

Hmm I'm still not convinced. Been playing around with that branch for a while and I have no idea what half the names even are, nor what underlying servers they're mapped to. I think tailwindcss is the culprit for many of these, but it's a general observation.

I only appended the filetypes to the original list of server names, so any additions are purely based on filetypes.

I think using filetypes as a method to discover LSPs is interesting, but perhaps not as a source for autocompletion.

I thought that was the whole point? Say a user doesn't know the name of the python server, or maybe they do know, now it doesn't matter anymore

image

Am I missing something obvious here?

What about allowing :LspInstall without any arguments, in which case it'd use the current buffer's (or perhaps all open buffers and make it interactive) filetype. We already kind of do this in the UI, where relevant LSPs are hoisted in the UI and receive a blue list icon.

I think that could also work, but you need a confirmation in case someone was expecting the UI panel instead. It might also be hard to guess that the command would allow this, so maybe you need a new command LspInstallBuf, but that's confusing because it sounds like you're only installing it for that buffer, so you're now stuck picking a descriptive-yet-not-too-long-name.

williamboman commented 2 years ago

I thought that was the whole point? Say a user doesn't know the name of the python server, or maybe they do know, now it doesn't matter anymore

Yep it is! I think the discussion is more about where to source these additional, aliased, entries from. In #232 I went with sourcing these through a new field provided by each server instance, whereas your branch directly uses lspconfig's filetype mapping. After giving both a try I still prefer not using filetypes, for reasons mentioned in my earlier comments. I think using filetypes actually has the opposite effect of adding more ambiguity, not less 😬 . But these are only my opinions, would be interesting to hear what others think before deciding which approach to use.

wookayin commented 2 years ago

but that's confusing because it sounds like you're only installing it for that buffer, so you're now stuck picking a descriptive-yet-not-too-long-name.

How about :LspInstall <lsp server name> and either

For the last one, we could consider providing completion for all the languages only if the prefix lang: is typed (lang: itself can appear in the first entry of the completion menu with no query, but not all the lang:xxx entries until then), in which case we can have a single command while taking advantage of rich command-line autocompletion (if any).

williamboman commented 2 years ago

I really don't want to add more variants of the :LspInstall command, I think that'd be solving problems that don't exist. Let's go with #232 for now. If it turns out to be a mess to maintain or if people are still struggling to discover servers we could give other approaches a try (like #233)