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

fix(lsp): create a proper way of removing items from skipped_servers #2503

Closed
wants to merge 1 commit into from

Conversation

abzcoding
Copy link
Member

Description

vim.tbl_map is unreliable (especially on mac), so just provide a simple function to remove item from map

for example you can add this to your config.lua

local tbl = require "lvim.utils.table"
tbl.remove(lvim.lsp.automatic_configuration.skipped_servers, "tailwindcss")

Fixes #2501

@kylo252
Copy link
Collaborator

kylo252 commented Apr 23, 2022

I must've copied the snippet wrong somewhere, because it should've been using vim.tbl_filter, but either of them don't mutate the original table, so the full syntax would end up looking like this

lvim.lsp.automatic_configuration.skipped_servers = vim.tbl_filter(function(s)
  return s ~= "tailwindcss"
end, lvim.lsp.automatic_configuration.skipped_servers)

which as you can see, is not that easy to read/follow 🥲

I've been contemplating adding a function similar to what you have created, but instead have it directly accessible

---remove a server or list of servers from automatic configuration's skiplist
---@param s string|table can be a string or a list of strings, e.g. "tailwindcss" or {"emmet_ls", "eslint"}
local function unskip(s)
  if type(s) == "string" then
    s = { s }
  end
  for _, item in ipairs(s) do
    tbl_remove(lvim.lsp.automatic_configuration.skipped_servers, item)
  end
end
lvim.lsp.automatic_configuration.unskip = unskip -- could instead be called "enable" or something

@abzcoding
Copy link
Member Author

with the function that you suggested, we would have three categories

  • skipped servers
  • unskipped server
  • we don't care servers

which is a bit weird
I believe having a skipped list and then removing items from it makes more sense, but it's up to you.

@kylo252
Copy link
Collaborator

kylo252 commented Apr 23, 2022

I was thinking of it more of a helper, but you're right, it's adding to the confusion instead of helping 😄

One thing I also realized is that removing servers from the skipped list was only necessary when the installer wasn't allowed to run for them, but that has changed in 5edbd91 (#2243), so it's basically enough to add this call somewhere:

require("lvim.lsp.manager").setup("tailwindcss", {})

pros

  • doesn't require any cache reset
  • ability to customize the setup opts

cons

  • the server still shows under skipped servers in LvimInfo
  • ??

@abzcoding
Copy link
Member Author

I was thinking of it more of a helper, but you're right, it's adding to the confusion instead of helping 😄

One thing I also realized is that removing servers from the skipped list was only necessary when the installer wasn't allowed to run for them, but that has changed in 5edbd91 (#2243), so it's basically enough to add this call somewhere:

require("lvim.lsp.manager").setup("tailwindcss", {})

pros

  • doesn't require any cache reset
  • ability to customize the setup opts

cons

  • the server still shows under skipped servers in LvimInfo
  • ??

hmm, it just got interesting 😅
not sure which route to go tbh, maybe keep this open for a bit

@kylo252
Copy link
Collaborator

kylo252 commented Apr 23, 2022

hmm, it just got interesting 😅 not sure which route to go tbh, maybe keep this open for a bit

as a quick and easy solution, I would remove the snippet entirely from config.example.lua and replace it with a commented call to one of the popular servers that are skipped, e.g. tailwindcss/emmet_ls/etc

-- all ts/js servers are skipped by default except for tsserver
require("lvim.lsp.manager").setup("tailwindcss", {})

and then fix this snippet on the site https://www.lunarvim.org/troubleshooting/#is-it-overriden

lvim.lsp.automatic_configuration.skipped_servers = vim.tbl_filter(function(s)
  return s ~= "tailwindcss"
end, lvim.lsp.automatic_configuration.skipped_servers)

@abzcoding
Copy link
Member Author

so which one do you prefer?

lvim.lsp.automatic_configuration.skipped_servers = vim.tbl_filter(function(s)
  return s ~= "tailwindcss"
end, lvim.lsp.automatic_configuration.skipped_servers)

or

local tbl = require "lvim.utils.table"
tbl.remove(lvim.lsp.automatic_configuration.skipped_servers, "tailwindcss")

?

@kylo252
Copy link
Collaborator

kylo252 commented Apr 24, 2022

so which one do you prefer?

I'll try to get some opinions on discord about this.

Another potential contender is implementing a tbl.remove_if which should be a little more flexible than tbl.remove and fits more with the theme of table helpers that take a predicate, here's an implementation for it https://github.com/aiq/luazdf/blob/0517286dcbe90d44ceeb05b22c143fa55703d1e9/arr/removeif/removeif.lua

@namimo
Copy link

namimo commented Jul 17, 2022

Is there a way to skip multiple servers. It would be nice to have something like {"tailwindcss", "eslint", "emmet_ls"}.

I used to do require("lvim.lsp.manager").setup("tailwindcss") but after clean install it stopped working.

@kylo252
Copy link
Collaborator

kylo252 commented Jul 17, 2022

Is there a way to skip multiple servers. It would be nice to have something like {"tailwindcss", "eslint", "emmet_ls"}.

I used to do require("lvim.lsp.manager").setup("tailwindcss") but after clean install it stopped working.

do you mean for un-skipping? because calling the setup('tailwindcss')is enough,

either way, it's just a simple lua table that you can alter however you want

:lua =lvim.lsp.automatic_configuration.skipped_servers

@namimo
Copy link

namimo commented Aug 12, 2022

This is not working:

require("lvim.lsp.manager").setup("tailwindcss")

This is working:

local unskipServers = { "tailwindcss", "eslint" }
for _, item in ipairs(unskipServers) do
	lvim.lsp.automatic_configuration.skipped_servers = vim.tbl_filter(function(server)
		return server ~= item
	end, lvim.lsp.automatic_configuration.skipped_servers)
end

The bad thing about this is every project now runs this server whether or not tailwind/eslint is configured in the project.
Tailwind is fine but eslint pops this message [lspconfig] unable to find eslint library every time i open a file. And if I add deno to the list node project confilcts with deno errors. Shouldn't it check for root_pattern.

I dont know either neovim config or lua and hacked around using the code above.

@kylo252
Copy link
Collaborator

kylo252 commented Aug 12, 2022

@namimo, the change to unblock tailwindcss was recent #2870, so you don't need to alter it anymore.

for eslint, you can use this something like

-- in ~/.config/lvim/after/ftplugin/typescript.lua
local util = require "lspconfig.util"
local opts = {
  root_dir = util.root_pattern(
    ".eslintrc",
    ".eslintrc.js",
    ".eslintrc.cjs",
    ".eslintrc.yaml",
    ".eslintrc.yml",
    ".eslintrc.json"
  ),
}
require("lvim.lsp.manager").setup("eslint", opts)

you could duplicate in other ftplugin files (that's how lunarvim works internally), or you could also set it up once in config.lua.

@lvimuser
Copy link
Collaborator

lvimuser commented Sep 4, 2022

Should we close this due to #2887 ?
I'm not in favor of adding another function, removing items from a table is straightforward...

@danielo515
Copy link
Contributor

the root cause of tis was that the tbl_map was not properly understood/used. I never had a single reliability problem with it, and I worked already on several macs (both x86 and M1).
Adding another function that does something is very simple to do with native functions seems like a bad idea

-- @param t The table
-- @param value The item to remove
-- @return True if item was removed, false otherwise
function Table.remove(t, value)
Copy link
Contributor

Choose a reason for hiding this comment

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

Mutation a function input is a bad idea. Built-in table filter has a better API in my opinion. Creating a new table and returning that is the best and safest way to proceed.

@abzcoding abzcoding closed this Sep 20, 2022
@abzcoding abzcoding deleted the fix/lsp-remove branch October 22, 2022 14:33
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.

Can't remove server from the skipped list
5 participants