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: don't add the disabled plugins to plugins list. #1079

Closed
wants to merge 2 commits into from

Conversation

ayamir
Copy link
Owner

@ayamir ayamir commented Nov 26, 2023

Signed-off-by: ayamir <lgt986452565@gmail.com>
@ayamir ayamir linked an issue Nov 26, 2023 that may be closed by this pull request
3 tasks
@YuCao16
Copy link
Contributor

YuCao16 commented Nov 26, 2023

#1077 @Jint-lzxy @CharlesChiuGit @YuCao16

It works, thanks.

By the way, I noticed that all the user plugins' config are stored in user/configs/*.lua, but since I got too many additional plugins, the user/configs/ directory got messy, can we introduce the structures just as the /modules/configs/, for example user/configs/ui/*.lua?

Copy link
Collaborator

@CharlesChiuGit CharlesChiuGit left a comment

Choose a reason for hiding this comment

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

LGTM

@YuCao16
Copy link
Contributor

YuCao16 commented Nov 26, 2023

@ayamir @CharlesChiuGit
I found a problem, this method seems not work for dependencies plugin, for example nvim-treesitter/nvim-treesitter, I cannot disable the dependencies such as andymass/vim-matchup.

Let's say I don't use andymass/vim-matchup, and I need another dependencies anuvyklack/pretty-fold.nvim. So in user/plugins/editor, I add following code (not sure this is the correct way?):

editor["nvim-treesitter/nvim-treesitter"] = {
	lazy = true,
	build = function()
		if #vim.api.nvim_list_uis() ~= 0 then
			vim.api.nvim_command([[TSUpdate]])
		end
	end,
	event = "BufReadPre",
	config = require("editor.treesitter"),
	dependencies = {
                {" anuvyklack/pretty-fold.nvim" },
		{ "andymass/vim-matchup" },
		{ "mfussenegger/nvim-treehopper" },
		{ "nvim-treesitter/nvim-treesitter-textobjects" },
		{
			"abecodes/tabout.nvim",
			config = require("editor.tabout"),
		},
		{
			"windwp/nvim-ts-autotag",
			config = require("editor.autotag"),
		},
		{
			"NvChad/nvim-colorizer.lua",
			config = require("editor.colorizer"),
		},
		{
			"hiphish/rainbow-delimiters.nvim",
			config = require("editor.rainbow_delims"),
		},
		{
			"nvim-treesitter/nvim-treesitter-context",
			config = require("editor.ts-context"),
		},
		{
			"JoosepAlviste/nvim-ts-context-commentstring",
			config = require("editor.ts-context-commentstring"),
		},
	},
}

and set disabled_plugins = {"andymass/vim-matchup"}, but andymass/vim-matchup still enabled as current solution don't modify the dependencies.

Signed-off-by: ayamir <lgt986452565@gmail.com>
@ayamir
Copy link
Owner Author

ayamir commented Nov 27, 2023

@YuCao16 It will remove plugins in dependenices now.

@ayamir
Copy link
Owner Author

ayamir commented Nov 27, 2023

By the way, I noticed that all the user plugins' config are stored in user/configs/.lua, but since I got too many additional plugins, the user/configs/ directory got messy, can we introduce the structures just as the /modules/configs/, for example user/configs/ui/.lua?

It can be done in another PR.

Copy link
Collaborator

@Jint-lzxy Jint-lzxy left a comment

Choose a reason for hiding this comment

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

Hmm I'm a bit opposed to this PR b/c it not only adds a layer of complexity, but also addresses only one potential "edge use case". This is why folke said in this issue that he won't support including two plugins with the same (plugin) name but different repo names in the spec at the same time. Instead, he suggested that ppl should use the dev (or url) option, e.g.:

completion["nvimdev/lspsaga.nvim"] = {
	url = "https://example.com/path/to/ur/lspsaga.nvim.git",
	-- other configs
}

@Jint-lzxy
Copy link
Collaborator

Jint-lzxy commented Nov 27, 2023

By the way, I noticed that all the user plugins' config are stored in user/configs/*.lua, but since I got too many additional plugins, the user/configs/ directory got messy, can we introduce the structures just as the /modules/configs/, for example user/configs/ui/*.lua?

It can be done in another PR.

I think we already support this now:

editor["m4xshen/autoclose.nvim"] = {
	lazy = true,
	event = "InsertEnter",
	config = require("configs.editor.autoclose"), -- will require user/configs/editor/autoclose.lua
}

Let's say I don't use andymass/vim-matchup, and I need another dependencies anuvyklack/pretty-fold.nvim. So in user/plugins/editor, I add following code (not sure this is the correct way?):

Because lazy.nvim will also (internally) merge the plugin spec it manages, you only need to write:

editor["nvim-treesitter/nvim-treesitter"] = {
	dependencies = {
                { "anuvyklack/pretty-fold.nvim" },
		{ "andymass/vim-matchup", enabled = false },
	},
}

@ayamir
Copy link
Owner Author

ayamir commented Nov 27, 2023

Hmm I'm a bit opposed to this PR b/c it not only adds a layer of complexity, but also addresses only one potential "edge use case". This is why folke said in this issue that he won't support including two plugins with the same (plugin) name but different repo names in the spec at the same time. Instead, he suggested that ppl should use the dev (or url) option, e.g.:

completion["nvimdev/lspsaga.nvim"] = {
	url = "https://example.com/path/to/ur/lspsaga.nvim.git",
	-- other configs
}

But seems it requires users to change repo's config. And I think we should keep our API for users works correctly for all of use cases.

@Jint-lzxy
Copy link
Collaborator

Hmm I'm a bit opposed to this PR b/c it not only adds a layer of complexity, but also addresses only one potential "edge use case". This is why folke said in this issue that he won't support including two plugins with the same (plugin) name but different repo names in the spec at the same time. Instead, he suggested that ppl should use the dev (or url) option, e.g.:

completion["nvimdev/lspsaga.nvim"] = {
	url = "https://example.com/path/to/ur/lspsaga.nvim.git",
	-- other configs
}

But seems it requires users to change repo's config. And I think we should keep our API for users works correctly for all of use cases.

I just checked the implementation of lazy.nvim, and it appears that when several plugins with the same name don't include conflicting URLs, lazy will use the source declared by the last appended specification (i.e., the spec provided by the user). The following minimal example demonstrates this:

local root = vim.fn.fnamemodify("./.repro", ":p")

-- set stdpaths to use .repro
for _, name in ipairs({ "config", "data", "state", "cache" }) do
	vim.env[("XDG_%s_HOME"):format(name:upper())] = root .. "/" .. name
end

-- bootstrap lazy
local lazypath = root .. "/plugins/lazy.nvim"
if not vim.loop.fs_stat(lazypath) then
	vim.fn.system({
		"git",
		"clone",
		"--filter=blob:none",
		"--single-branch",
		"https://github.com/folke/lazy.nvim.git",
		lazypath,
	})
end
vim.opt.runtimepath:prepend(lazypath)

-- install plugins
local plugins = {
	"folke/tokyonight.nvim",
	-- add any other pugins here
	{ "nvimdev/lspsaga.nvim" },
	{ "ayamir/lspsaga.nvim", opts = {} },
}
require("lazy").setup(plugins, {
	root = root .. "/plugins",
})

vim.opt.termguicolors = true
vim.cmd([[colorscheme tokyonight]])

Save it as init.lua, and running it with nvim -u init.lua yields the following (possible) result:

  ● lspsaga.nvim 12.42ms ▷ start
      dir    /private/var/folders/jy/91xg_8j9169dydct2k4r7yw80000gn/T/tmp.chC1vsz66n/.repro/plugins/lspsaga.nvim
      url    https://github.com/ayamir/lspsaga.nvim
                                ^^^^^^
      branch main
      commit d3dfaea
      readme README.md
      help   |lspsaga.nvim.txt|

So IMO our loader has indeed done what it was supposed to do. The only thing missing here is we need to document this particular use case in the wiki - if users want to use their own fork(s) to overwrite the default one(s), they don’t need to include the base plugin(s) in disabled_plugins.

@ayamir
Copy link
Owner Author

ayamir commented Nov 29, 2023

OK, IMO this pr and #1077 can be closed now. @YuCao16

@ayamir ayamir closed this Nov 29, 2023
@ayamir ayamir deleted the fix/disable-plugins branch November 29, 2023 13:36
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.

"disabled_plugins" setting won't work for forked plugins
5 participants