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

user setting lsp list not replaced default config, but merge with default #972

Closed
2 tasks done
HATTER-LONG opened this issue Aug 26, 2023 · 6 comments
Closed
2 tasks done
Labels
enhancement New feature or request

Comments

@HATTER-LONG
Copy link
Contributor

Version confirmation

  • Confirm

Following prerequisites

  • Confirm

Neovim version

0.9.1

Branch info

main (Default/Latest)

Minimal (user) folder structure required to reproduce the issue

user/setting.lua

Minimal config with steps on how to reproduce the issue

settings["dap_deps"] = {
	"codelldb", 
	-- "delve", --  remove Go
	"python", 
}

Expected behavior

Will still install delve by default, this phenomenon exists in other settings lists.

I think it's related to the following implementations, vim.list_extend will merge list which default and user config.

But I didn't think of any good ways to deal it.

--- Function to recursively merge src into dst
--- Unlike vim.tbl_deep_extend(), this function extends if the original value is a list
---@paramm dst table @Table which will be modified and appended to
---@paramm src table @Table from which values will be inserted
---@return table @Modified table
local function tbl_recursive_merge(dst, src)
	for key, value in pairs(src) do
		if type(dst[key]) == "table" and type(value) == "function" then
			dst[key] = value()
		elseif type(dst[key]) == "table" and vim.tbl_islist(dst[key]) then
			vim.list_extend(dst[key], value)
		elseif type(dst[key]) == "table" and not vim.tbl_islist(dst[key]) then
			tbl_recursive_merge(dst[key], value)
		else
			dst[key] = value
		end
	end
	return dst
end

Add a block list setting It seems a bit redundant. Perhaps we don't need to specify any lsp by default.

Additional information

No response

@HATTER-LONG HATTER-LONG added the usage User-specific issues label Aug 26, 2023
@ayamir
Copy link
Owner

ayamir commented Aug 26, 2023

You are right. IMO it's better to replace the default configuration instead of merging them.
What do you think? @misumisumi @CharlesChiuGit @Jint-lzxy

@misumisumi
Copy link
Collaborator

misumisumi commented Aug 27, 2023

I'm doing a merge implementation to respect the existing settings as much as possible.
If you want to replace it completely, you can set it as follows.
It is easy to understand whether the default settings are maintained or completely replaced, so I think that the current state of using functions is fine.

settings["dap_deps"] = function()
    return {
        "codelldb", -- C-Family
        "python", -- Python (debugpy)
    }
end

@vollowx
Copy link

vollowx commented Aug 27, 2023

Can we provide a param default when merging the override settings like:

settings['lsp_servers'] = function (default)
  return { 'tsserver' }
end

This way, users can also merge them by calling vim.... inside the function while they can completely override them.

@misumisumi
Copy link
Collaborator

misumisumi commented Aug 27, 2023

Yeah.
Certainly, extensibility will increase if you can refer to the default settings.
We can implement it like this:

--- Function to recursively merge src into dst
--- Unlike vim.tbl_deep_extend(), this function extends if the original value is a list
---@paramm dst table @Table which will be modified and appended to
---@paramm src table @Table from which values will be inserted
---@return table @Modified table
local function tbl_recursive_merge(dst, src)
	for key, value in pairs(src) do
		if type(dst[key]) == "table" and type(value) == "function" then
			dst[key] = value(dst[key]) -- give default table as args
		elseif type(dst[key]) == "table" and vim.tbl_islist(dst[key]) then
			vim.list_extend(dst[key], value)
		elseif type(dst[key]) == "table" and not vim.tbl_islist(dst[key]) then
			tbl_recursive_merge(dst[key], value)
		else
			dst[key] = value
		end
	end
	return dst
end

If we want to change it, we should be able to refer to the default settings in M.load_plugin as well.

---@param plugin_name string @Module name of the plugin (used to setup itself)
---@param opts nil|table @The default config to be merged with
---@param vim_plugin? boolean @If this plugin is written in vimscript or not
---@param setup_callback? function @Add new callback if the plugin needs unusual setup function
function M.load_plugin(plugin_name, opts, vim_plugin, setup_callback)
...
				elseif type(user_config) == "function" then
					local user_opts = user_config(opts) -- change this line
					if type(user_opts) == "table" then
						setup_callback(user_opts)
					end
...
end

@HATTER-LONG
Copy link
Contributor Author

Using return functions is a good way to avoid it, but it's needed to improve some wiki info for it. 😊

@CharlesChiuGit
Copy link
Collaborator

I'm doing a merge implementation to respect the existing settings as much as possible. If you want to replace it completely, you can set it as follows. It is easy to understand whether the default settings are maintained or completely replaced, so I think that the current state of using functions is fine.

settings["dap_deps"] = function()
    return {
        "codelldb", -- C-Family
        "python", -- Python (debugpy)
    }
end

agree

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants