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

Beta-testing 'mini.deps' #689

Closed
echasnovski opened this issue Feb 11, 2024 · 58 comments
Closed

Beta-testing 'mini.deps' #689

echasnovski opened this issue Feb 11, 2024 · 58 comments

Comments

@echasnovski
Copy link
Owner

Please leave your feedback about new mini.deps module here. Feel free to either add new comment or positively upvote existing one.

Some things I am interested to find out (obviously, besides bugs):

  • Are available actions (install, update, clean, snapshot) enough?
  • Are available plugin specification fields enough?
  • Is configuration intuitive enough?
  • Are default values of settings convenient for you?
  • Is documentation and examples clear enough?

Thanks!

@echasnovski echasnovski pinned this issue Feb 11, 2024
@AlexKurisu
Copy link
Contributor

Hello! Thank you for developing another great plugin. Here's a few things I noticed:

  • Install instruction should also do vim.cmd.packadd("mini.nvim") after cloning the plugin (example init.lua already does this and without packadd initial install will fail)
  • Help tags are missing after adding both the plugin and other plugins with MiniDeps.add. Is this by design?

@echasnovski
Copy link
Owner Author

Thanks for the feedback!

* Install instruction should also do `vim.cmd.packadd("mini.nvim")` after cloning the plugin (example init.lua already does this and without `packadd` initial install will fail)

Yeah, it was the late addition in example 'init.lua' after I the release, so forgot to add it in other places. Should be fixed on latest main.

* Help tags are missing after adding both the plugin and other plugins with `MiniDeps.add`. Is this by design?

All helptags should be generated during installation in MiniDeps.add(). For example, I can see tags from 'nvim-treesitter' right after first launch of 'nvim-treesitter'.
Helptags of 'mini.nvim' itself will now also be generated on first installation. Thanks for noticing!

@AlexKurisu
Copy link
Contributor

AlexKurisu commented Feb 11, 2024

For example, I can see tags from 'nvim-treesitter' right after first launch of 'nvim-treesitter'

Tried this on new config and now I can see them too. Sorry.

@wroyca
Copy link
Contributor

wroyca commented Feb 11, 2024

Great module! Right on time for my desire to stray away from lazy.nvim. :)

Are available plugin specification fields enough?

As we are in Git territory, something that would be helpful is a patch field to leverage Git apply and git-send-email functionality, among others. The general idea would be to git restore cloned modules and then point to a .patch file for respective modules and/or let Git's metadata take care of locating and applying a patch set. What do you think?

@echasnovski
Copy link
Owner Author

As we are in Git territory, something that would be helpful is a patch field to leverage Git apply and git-send-email functionality, among others. The general idea would be to git restore cloned modules and then point to a .patch file for respective modules and/or let Git's metadata take care of locating and applying a patch set. What do you think?

That sounds both interesting and not too minimal. Right now I think that there are at least way to try and achieve similar workflow:

  • The obvious one: clone target repositories and apply patches there. This gives more freedom to users.
  • Utilize post_intsall and post_checkout hooks to apply patches manually. With something like vim.fn.system('git am ~/patches/patch.patch && git add * && git checkout -m "Apply patches"').

The first one looks more appropriate, but the second one looks more doable.

@wroyca
Copy link
Contributor

wroyca commented Feb 11, 2024

To add to the discussion, I believe it's out-of-scope for a plugin manager to checkout, that is, it should apply a patch locally, so ideally what we really want is something like:

vim.fn.system('git restore ~/.local/data/site/pack/deps/opt/foobar')
vim.notify('applying 000-foobar.patch for foobar')
vim.fn.system('git am ~/patches/000-foobar.patch')

@echasnovski
Copy link
Owner Author

To add to the discussion, I believe it's out-of-scope for a plugin manager to checkout, that is, it should apply a patch locally,

If I understood correctly and it is about how plugin manager internally should utilize Git, then to me it looks more complicated then actually checking out which allows for a manageable way to compute log of changes between commits. 'mini.deps' uses detached HEAD so using checkout is basically: get target hash and checkout. No merge conflicts.

@yamin-shihab
Copy link
Contributor

From the README/help file:

By default "opt" subdirectory is used to install optional plugins which are loaded on demand with MiniDeps.add(). Non-optional plugins in "start" subdirectory are supported but only if moved there manually after initial install.

Would it be possible to get an option for individual or all packages to have them be set up in the "start" directory instead of having to manually move plugin directories there from "opt"?

@AlexKurisu
Copy link
Contributor

AlexKurisu commented Feb 11, 2024

I noticed another thing: functions scheduled with MiniDeps.later trigger only after the buffer has appeared. Is this how it should work? This breaks nvim-lspconfig, nvim-tree and, possibly, every plugin that relies on events <= BufEnter

@AlexKurisu
Copy link
Contributor

I use neovim nightly btw

@echasnovski
Copy link
Owner Author

Would it be possible to get an option for individual or all packages to have them be set up in the "start" directory instead of having to manually move plugin directories there from "opt"?

It was a deliberate choice as (currently) I think that adding this will only bring confusion. The main issue being that "start" plugins will still be added to session even if add() call in the 'init.lua' is removed. As the main suggested approach to using plugins is by adding add() call to 'init.lua', this will confuse people not deeply familiar with how built-in packages work.

Would you mind elaborating on the importance of having this as a possible field in plugin specification? To me intentional usage of "start" will lead to a one time manual directory move for "permanent" plugins after their initial install. Which doesn't look like much of a hurdle.

@echasnovski
Copy link
Owner Author

I noticed another thing: functions scheduled with MiniDeps.later trigger only after the buffer has appeared. Is this how it should work? This breaks nvim-lspconfig, nvim-tree and, possibly, every plugin that relies on events <= BufEnter

The later() function has similar effect as vim.schedule() (but deferred by 1 ms each). So they will be scheduled to be executed in the next "event loop".

The main help explanation is at :h lua-event-loop and :h vim.schedule(). The more simplistic explanation is that function in later() will be executed after any code scheduled to be executed immediately is executed. As most of the events execute their autocommands immediately, it should be taken into account.

I am not sure why using later() will break plugin relying on BufEnter, though. If some plugin is implemented in a way that it has to set something up during startup, then lazy loading it is not a good idea. Also using later() inside BufEnter event and expecting it to be executed immediately is also not how things are supposed to work.

I have 'nvim-lspconfig' set up in later() and it works fine, but mostly because I use 'mini.starter' and I very rarely need LSP server attached exactly during startup. I am not sure how 'nvim-tree' can be affected, though.

@Ernest1338
Copy link

Ernest1338 commented Feb 12, 2024

I'm not sure if this is related to what @AlexKurisu explained but I can't get nvim-cmp to work with later(). I get errors that say: unknown source names (When executing :CmpStatus). Maybe it has something to do with the depends functionality which I use to specify the cmp sources? Minimal reproduction config:

-- mini.deps setup

later(function()
    add({source = "hrsh7th/nvim-cmp", depends = {"hrsh7th/cmp-buffer"}})

    local cmp = require("cmp")
    cmp.setup({
        sources = cmp.config.sources({
            { name = 'buffer' },
        })
    })
end)

Change the later to now and it works as expected. I'm using stable but also tested on nightly.

@AlexKurisu
Copy link
Contributor

AlexKurisu commented Feb 12, 2024

I am not sure why using later() will break plugin relying on BufEnter, though

If added in later() then plugin gets added after BufEnter event already fired

@yamin-shihab
Copy link
Contributor

yamin-shihab commented Feb 12, 2024

Would it be possible to get an option for individual or all packages to have them be set up in the "start" directory instead of having to manually move plugin directories there from "opt"?

It was a deliberate choice as (currently) I think that adding this will only bring confusion. The main issue being that "start" plugins will still be added to session even if add() call in the 'init.lua' is removed. As the main suggested approach to using plugins is by adding add() call to 'init.lua', this will confuse people not deeply familiar with how built-in packages work.

Would you mind elaborating on the importance of having this as a possible field in plugin specification? To me intentional usage of "start" will lead to a one time manual directory move for "permanent" plugins after their initial install. Which doesn't look like much of a hurdle.

Doesn't the default configuration put mini.nvim into "start" instead of "opt", unlike every other plugin? At this point it might be inconsistency bothering me, but this means mini.nvim will still be started without an add().

@echasnovski
Copy link
Owner Author

Doesn't the default configuration put mini.nvim into "start" instead of "opt", unlike every other plugin? At this point it might be inconsistency bothering me, but this means mini.nvim will still be started without an add().

You can put it in "opt" and then manually call vim.cmd('packadd mini.nvim') right after the if-then which ensures it is present. In fact, that was the case just several days ago.

I moved it to "start" because if put in "opt" it would require another add('mini.nvim') call to make it recognizable by 'mini.deps'. If omitted, 'mini.deps' will delete the whole 'mini.nvim' on the next :DepsClean. Which is suboptimal.

Together with possible confusion from two calls adding 'mini.nvim', I decided to put it to "start". If anything, it is a somewhat special plugin in this case.

@echasnovski
Copy link
Owner Author

I'm not sure if this is related to what @AlexKurisu explained but I can't get nvim-cmp to work with later(). I get errors that say: unknown source names (When executing :CmpStatus). Maybe it has something to do with the depends functionality which I use to specify the cmp sources? Minimal reproduction config:

-- mini.deps setup

later(function()
    add({source = "hrsh7th/nvim-cmp", depends = {"hrsh7th/cmp-buffer"}})

    local cmp = require("cmp")
    cmp.setup({
        sources = cmp.config.sources({
            { name = 'buffer' },
        })
    })
end)

Change the later to now and it works as expected. I'm using stable but also tested on nightly.

I did some investigating. The problem here is that cmp-buffer registers its source in the after/plugin/cmp_buffer.lua file. So in order for it to work, it should be present in 'runtimepath'.

Now, for some reason, :packadd (which add() uses internally) does not add (and execute) any 'after/plugin' directory to 'runtimepath' when called inside later() (while still properly executing the normal 'plugin'). But it works inside now(). I'll look into it more carefully, as it is either some simple mistake on my side or a known issue in Vim/Neovim.

In the meantime, adding cmp.register_source('buffer', require('cmp_buffer')) line in the example after local cmp = require("cmp") solves the issue.

For what it's worth, it is a known issue in 'nvim-cmp' (there are couple similar issues out there).

@echasnovski
Copy link
Owner Author

@Ernest1338, so this indeed proved to be kind of both: simple-ish mistake and a known issue in Vim. In fact, it is a vim/vim#1994.

add() uses :packadd under the hood (as it should for built-in packages) which only sources 'plugin/' files and not 'after/plugin/'. It just so happens that 'nvim-cmp' sources for some reason uses the latter instead of the former. This manifested into the issue you experienced: now() executed immediately resulting into 'after/plugin/' be executed as part of Neovim startup, while later() did not.

It should now work on latest main. I've tested with example 'init.lua'.


For what it is worth, I think it would simplify life for plugin managers if plugins stopped using 'after/plugin/' in favor of 'plugin/'. I don't see a valid reason for preferring former over latter. I'd suggest opening an issue in 'nvim-cmp' (and/or its sources) suggesting to not use 'after/plugin/'.

@echasnovski
Copy link
Owner Author

echasnovski commented Feb 12, 2024

I am not sure why using later() will break plugin relying on BufEnter, though

If added in later() then plugin gets added after BufEnter event already fired

@AlexKurisu, that can only happen if BufEnter is already scheduled to be triggered. Like during startup or as a result of code before or after later().

Would you mind providing a reproducible example with the issue description you are thinking about?

@AlexKurisu
Copy link
Contributor

a reproducible example with the issue description you are thinking about
Sure.

Code
-- Bootstrap mini.nvim
local path_package = vim.fn.stdpath("data") .. "/site/"
local mini_path = path_package .. "pack/deps/start/mini.nvim"
if not vim.loop.fs_stat(mini_path) then
	vim.cmd([[echo 'Installing `mini.nvim`' | redraw]])
	local clone_cmd = {
		"git",
		"clone",
		"--filter=blob:none",
		"https://github.com/echasnovski/mini.nvim",
		mini_path,
	}
	vim.fn.system(clone_cmd)
	vim.cmd([[packadd mini.nvim | helptags ALL]])
end

-- Setup mini.deps
require("mini.deps").setup({ path = { package = path_package } })

local now = MiniDeps.now
local later = MiniDeps.later

now(function()
	vim.api.nvim_create_autocmd("BufEnter", {
		callback = function()
			vim.print("Handler 1 (added in now()) fired!")
		end,
	})
end)

later(function()
	vim.api.nvim_create_autocmd("BufEnter", {
		callback = function()
			vim.print("Handler 2 (added in later()) fired!")
		end,
	})
end)
later(function()
	MiniDeps.add("neovim/nvim-lspconfig")
	require("lspconfig").lua_ls.setup({})
end)

This will print just Handler 1 (added in now()) fired! and lua_ls will not start automatically when you open Lua scripts with neovim (like nvim <path>)

@AlexKurisu
Copy link
Contributor

If this is how it should work, then please mention this in documentation

@echasnovski
Copy link
Owner Author

This will print just Handler 1 (added in now()) fired! and lua_ls will not start automatically when you open Lua scripts with neovim (like nvim <path>)

Yes, this works as intended. Precisely for the reason described earlier: function in later() gets executed not earlier than next event loop. As all BufEnter during startup are executed in the same event loop, autocommand with "Handler 2 (added in later()) fired!" is not yet created. But it still gets created, as you can see by executing :enew which prints both messages (from now() and later()).

If this is how it should work, then please mention this in documentation

I'll think about an appropriate description (if any).

@hosaka
Copy link

hosaka commented Feb 13, 2024

My git doesn't have a --also-filter-submodules for the git clone subcommand, resulting in:

(mini.deps) Installing `nvim-web-devicons`
(mini.deps) Error in `nvim-web-devicons` during installing plugin
PROCESS EXITED WITH ERROR CODE 129
error: unknown option `also-filter-submodules'

❯ git --version
git version 2.34.1

@echasnovski
Copy link
Owner Author

My git doesn't have a --also-filter-submodules for the git clone subcommand, resulting in:

Thanks for the feedback. That's exactly what beta-testing is about.

As for this particular case, the --also-filter-submodules option was added almost exactly two years ago and is present in a 2.36.0 (released almost two years ago). I'd consider this time period enough to actually use the option without any special handling.

So I am afraid the suggestion here would be to ask to update Git to at least 2.36.0 or better yet the latest release 2.43.0.

@hosaka
Copy link

hosaka commented Feb 14, 2024

@echasnovski yeah that's totally fair, I thought it's worth pointing out since my git was installed from Ubuntu 22.04.3 LTS repo and was not recent enough (current version is 2.34.1). I've added a git PPA and installed the latest version. Perhaps a note in the readme/help file would be useful. Great work btw, I was looking forward to using mini.deps!

@echasnovski
Copy link
Owner Author

@echasnovski yeah that's totally fair, I thought it's worth pointing out since my git was installed from Ubuntu 22.04.3 LTS repo and was not recent enough (current version is 2.34.1). I've added a git PPA and installed the latest version. Perhaps a note in the readme/help file would be useful. Great work btw, I was looking forward to using mini.deps!

Ow, that's a more than a valid reason to make 'mini.deps' work on 2.34.1. Thanks for mentioning that, I was not aware! I'll make it work with 2.34.1 by dropping that option there.

@AlexKurisu
Copy link
Contributor

Yet another note here: what do you think about passing path to plugin's directory to hooks? Currently it's kinda painful to run hooks that execute system commands (generally make <target>)

@eckig
Copy link

eckig commented Feb 16, 2024

As far as I can see. it is not in the example init.lua. But adding it to the session works. Thanks.

@eglimi
Copy link

eglimi commented Feb 16, 2024

I also ported from lazy and all works great.

One thing I noticed is that the confirmation report of DepsUpdate is a bit verbose. I have ~20 plugins and with the current output, I need to scroll quite a bit to see which ones have pending updates. Especially if one of the plugins has many commits.

I would prefer a more compact output, maybe with a fold to reveal the extra information currently displayed?

This is just a cosmetic change. Overall, I'm very happy with this and other mini modules and port more and more plugins to it. The quality is great, and I also appreciate the work and care you put into the documentation. Thank you!

@echasnovski
Copy link
Owner Author

echasnovski commented Feb 16, 2024

I also ported from lazy and all works great.

🚀

One thing I noticed is that the confirmation report of DepsUpdate is a bit verbose. I have ~20 plugins and with the current output, I need to scroll quite a bit to see which ones have pending updates. Especially if one of the plugins has many commits.

I would prefer a more compact output, maybe with a fold to reveal the extra information currently displayed?

This is just a cosmetic change. Overall, I'm very happy with this and other mini modules and port more and more plugins to it. The quality is great, and I also appreciate the work and care you put into the documentation. Thank you!

I feel you here. Initially during development the confirmation buffer always had order as in session. Very quickly it started to feel too verbose and now plugins with errors and pending updates are listed first.

That said, adding folds by default is not very user friendly because they are certainly underused by most users. You can set up custom folds manually by utilizing 'minideps-confirm' filetype. For example, create a '~/.config/nvim/after/ftplugin/minideps-confirm.lua' with the following content:

vim.wo.foldmethod = 'expr'
vim.wo.foldexpr = 'match(getline(v:lnum), "^+++\\\\|^---\\\\|^!!!") < 0'

This fold anything that is not a title for plugin data. Edit: folds are now implemented in main branch, but are unfolded initially.

But make sure to update to latest main, as your question brought some issue which was fixed several minutes ago (eventual target window was not current when FileType was triggered, so vim.wo did not work). So thanks for that :)

@bdillahu
Copy link

bdillahu commented Feb 17, 2024

Having an issue with mini.clue... setup code worked fine with lazy.nvim, but with mini.deps I get:

(mini.deps) There were errors during two-stage execution:

/Users/bdillahu/.config/nvim/init.lua:434: attempt to index global 'miniclue' (a nil value)

Config section in question is (if I comment out the "clues" section all is happy):

 now(function() require('mini.clue').setup({
          triggers = {
            -- Leader triggers
            { mode = 'n', keys = '<Leader>' },
            { mode = 'x', keys = '<Leader>' },

            -- Built-in completion
            { mode = 'i', keys = '<C-x>' },

            -- `g` key
            { mode = 'n', keys = 'g' },
            { mode = 'x', keys = 'g' },

            -- Marks
            { mode = 'n', keys = "'" },
            { mode = 'n', keys = '`' },
            { mode = 'x', keys = "'" },
            { mode = 'x', keys = '`' },

            -- Registers
            { mode = 'n', keys = '"' },
            { mode = 'x', keys = '"' },
            { mode = 'i', keys = '<C-r>' },
            { mode = 'c', keys = '<C-r>' },

            -- Window commands
            { mode = 'n', keys = '<C-w>' },

            -- `z` key
            { mode = 'n', keys = 'z' },
            { mode = 'x', keys = 'z' },
          },

          clues = {
            -- Enhance this by adding descriptions for <Leader> mapping groups
            miniclue.gen_clues.builtin_completion(),
            miniclue.gen_clues.g(),
            miniclue.gen_clues.marks(),
            miniclue.gen_clues.registers(),
            miniclue.gen_clues.windows(),
            miniclue.gen_clues.z(),
            { mode = 'n', keys = '<Leader>h', desc = '+Hop' },
            { mode = 'n', keys = '<Leader>m', desc = '+MiniMap' },
            { mode = 'n', keys = '<Leader>s', desc = '+Search' },
            { mode = 'n', keys = '<Leader>z', desc = '+Zettlekasten' },
            { mode = 'n', keys = '<Leader>l', desc = '+LSP' },
            { mode = 'n', keys = '<Leader>G', desc = '+Git' },
          },
        }) 

Probably just me not understanding how to do something, but...

Thanks!

@echasnovski
Copy link
Owner Author

Having an issue with mini.clue... setup code worked fine with lazy.nvim, but with mini.deps I get:

(mini.deps) There were errors during two-stage execution:

/Users/bdillahu/.config/nvim/init.lua:434: attempt to index global 'miniclue' (a nil value)

Well, the error message is quite informative here and probably the result of not full copy-paste. At line 434 there is a miniclue variable used, which was not defined before. Add the separate line local miniclue = require('mini.clue') before calling require('mini.clue').setup() and it should work.

@echasnovski
Copy link
Owner Author

I would prefer a more compact output, maybe with a fold to reveal the extra information currently displayed?

This is just a cosmetic change. Overall, I'm very happy with this and other mini modules and port more and more plugins to it. The quality is great, and I also appreciate the work and care you put into the documentation. Thank you!

@eglimi, after spending some time thinking and tinkering, I did find having folds a good user experience. Thanks again for bringing this up!

Unfortunately, too few people actually use them, so I opted out for a unfolded initial view with an extra help line describing that folds can be used.

So they should now be usable on main branch. The actual foldexpr gives a bit nicer view. Would you mind checking it out and tell me you impressions/issues/etc.? Also, please make sure that code from this advice does not affect new folds.

@bdillahu
Copy link

Having an issue with mini.clue... setup code worked fine with lazy.nvim, but with mini.deps I get:

(mini.deps) There were errors during two-stage execution:

/Users/bdillahu/.config/nvim/init.lua:434: attempt to index global 'miniclue' (a nil value)

Well, the error message is quite informative here and probably the result of not full copy-paste. At line 434 there is a miniclue variable used, which was not defined before. Add the separate line local miniclue = require('mini.clue') before calling require('mini.clue').setup() and it should work.

Thanks, you nailed it :-)

@abeldekat
Copy link

I noticed another thing: functions scheduled with MiniDeps.later trigger only after the buffer has appeared. Is this how it should work? This breaks nvim-lspconfig, nvim-tree and, possibly, every plugin that relies on events <= BufEnter

@AlexKurisu,

For nvim-lspconfig, when loaded using later(), you can add:

    --- your lsp setup code
    vim.cmd("LspStart") --

Command LspStart is lenient. If there is no lsp to start, nothing happens.

@abeldekat
Copy link

Hello @echasnovski,

I was beta testing mini.deps whilst it was still in development.

At the time, there was a use-case I had to implemenent myself.

There are clusters of plugins I only want to load on demand using keys. For example, test and dap.
It's quite easy to do so:

later(function()
  local sources = {
    "mason-nvim-dap.nvim",
    "nvim-dap-ui",
    "nvim-dap-virtual-text",
    "nvim-dap-python",
    "nvim-dap",
  }

  Util.defer.on_keys(function()
    for _, source in ipairs(sources) do
      add(source)
    end
    require("ak.config.lang.debugging")
  end, "<leader>dL", "Load dap")
end)

Util.defer.on_keys is a simple function, creating the keymap to execute the callback:

function M.on_keys(cb, keys, desc)
  keys = type(keys) == "string" and { keys } or keys
  for _, key in ipairs(keys) do
    vim.keymap.set("n", key, function()
      vim.keymap.del("n", key)
      cb()
      vim.api.nvim_input(vim.api.nvim_replace_termcodes(key, true, true, true))
    end, { desc = desc, silent = true })
  end
end

I also used a similar pattern for the filetype event, loading some markdown plugins.

The problem: By doing so, on startup, those plugins are unknown to mini.deps and will not be updated.
I solved this by registering these plugins manually, allowing for a trigger to load them all. When I wanted to update all plugins, I used an environment variable, ALL_DEPS, when starting Neovim with the intent to upgrade all plugins.

Perhaps, it is possible to have a register method, complementing the add method. The register method would not perform a packadd. The plugin would just be taken into account when mini.deps starts its procedures.

You wrote:

Some things I am interested to find out (obviously, besides bugs)

I can say yes on all points... In addition, I think the code is concise and very readable.

@echasnovski
Copy link
Owner Author

Some things I am interested to find out (obviously, besides bugs)

I can say yes on all points... In addition, I think the code is concise and very readable.

Thanks for the feedback and early adoption!

The problem: By doing so, on startup, those plugins are unknown to mini.deps and will not be updated. I solved this by registering these plugins manually, allowing for a trigger to load them all. When I wanted to update all plugins, I used an environment variable, ALL_DEPS, when starting Neovim with the intent to upgrade all plugins.

Perhaps, it is possible to have a register method, complementing the add method. The register method would not perform a packadd. The plugin would just be taken into account when mini.deps starts its procedures.

I don't think 'mini.deps' will gain support (direct or indirect) for any other type of lazy-loading besides it current has. Personally I don't really want to encourage any complex lazy loading (i.e. anything other than the documented two-stage one).

What I can suggest trying is to use add({ ... }, { bang = true }) for plugins which you want to "register" but don't want to load any 'plugin' / 'after/plugin' scripts. Yes, this would double the amount of add() calls for these plugins, but I think this is a good compromise here.

@abeldekat
Copy link

I don't think 'mini.deps' will gain support (direct or indirect) for any other type of lazy-loading besides it current has. Personally I don't really want to encourage any complex lazy loading (i.e. anything other than the documented two-stage one).

That makes perfect sense.
Thank you for the bang suggestion! That would be a good compromise indeed.

@eglimi
Copy link

eglimi commented Feb 19, 2024

@eglimi, after spending some time thinking and tinkering, I did find having folds a good user experience. Thanks again for bringing this up!

Unfortunately, too few people actually use them, so I opted out for a unfolded initial view with an extra help line describing that folds can be used.

That seems reasonable and this update works well for me. Thank you for considering and implementing this!

So they should now be usable on main branch. The actual foldexpr gives a bit nicer view. Would you mind checking it out and tell me you impressions/issues/etc.?

I like it 😄
One issue I have is that I would like to set the initial foldlevel to 0, i.e. close all folds by default. This didn't work for me with an autocmd or a ftplugin entry, it seems the plugin overwrites this with 999 later. Is there a way to overwrite it?

Also, please make sure that code from this advice does not affect new folds.

It didn't conflict with manual foldexpr from the advice for me. But after the update, I removed the ftplugin entry.

@echasnovski
Copy link
Owner Author

One issue I have is that I would like to set the initial foldlevel to 0, i.e. close all folds by default. This didn't work for me with an autocmd or a ftplugin entry, it seems the plugin overwrites this with 999 later. Is there a way to overwrite it?

Ah, yes. This is because filetype was set before setlocal foldlevel=999. This should be resolved on latest main. Thanks for noticing!

I would suggest achieving this (showing folds initially) with vim.cmd('setlocal foldlevel=0') (that's what I did) as buffer-window global-local options not always work as expected with vim.wo.

It didn't conflict with manual foldexpr from the advice for me. But after the update, I removed the ftplugin entry.

Well, with latest main it would have :)

@abeldekat
Copy link

abeldekat commented Feb 20, 2024

Hello @echasnovski,

I gathered some extra feedback and questions

Very minor:

In the browser, enter https://github.com/echasnovski/mini.deps
In the Overview, click on link named example init.lua file. Result: File not found.
The file is found when the navigation starts with https://github.com/echasnovski/mini.nvim

Hooks
I do not completely understand the different purposes of the hooks pre/post_install and pre/post_checkout

In the docs:

Hooks to call before/after plugin is created/changed.
...
<post_install> - after creating plugin directory.

Does this mean that the post_install hook will only trigger on initial install, and not on subsequent updates? Is there a use case where both post_install and post_checkout are needed? I currently use this fragment, as I am not sure:

  local function make_fzf_native(path)
    vim.cmd("lcd " .. path)
    vim.cmd("!make -s")
    vim.cmd("lcd -")
  end
  add({
    source = "nvim-telescope/telescope.nvim",
    depends = {
      {
        source = "nvim-telescope/telescope-fzf-native.nvim",
        hooks = {
          post_install = function(params) make_fzf_native(params.path) end,
          post_checkout = function(params) make_fzf_native(params.path) end,
        },
      },
    },
  })
  require("ak.config.editor.telescope")

Installation
Is it necessary to also add() mini.nvim or mini.deps to the session, when cloned initially using the example init.lua file. EDIT I assume that mini.deps, when updating, also takes the plugins in start into account.

EDIT
Notify
On initial install, using cmdheight=0, without a notification plugin, there is no feedback at all. Using 80 plugins, it takes quite some time for the screen to become active, allowing for :messages to check the installation.
Adding mini.notify of course fixes the notifications. I might change my opinion on not using notifications. Very nice!

@echasnovski
Copy link
Owner Author

In the browser, enter https://github.com/echasnovski/mini.deps
In the Overview, click on link named example init.lua file. Result: File not found.
The file is found when the navigation starts with https://github.com/echasnovski/mini.nvim

Ah, indeed, that is an issue with dual distribution script. I'll look into the best way to solve this.

I do not completely understand the different purposes of the hooks pre/post_install and pre/post_checkout
...
Does this mean that the post_install hook will only trigger on initial install, and not on subsequent updates? Is there a use case where both post_install and post_checkout are needed?

Yes, *_install will be called only on initial install to set something up only one time (like system dependencies, etc.) so as to not do this on every checkout/update. It does look like a bit of an overkill, as most (all?) others plugin manager have only single hooks ("build", "run", "do", etc.) which is called both on initial install and checkout/update. It might have been an over-engineering oversight, but it is what it is now.

And yes, if you need to make fzf-native both after initial install and every checkout/update, this is the way to go. I'd rather make make_fzf_native to itself accept params so that hooks becomes { post_install = make_fzf_native, post_checkout = make_fzf_native }.

Installation
Is it necessary to also add() mini.nvim or mini.deps to the session, when cloned initially using the example init.lua file

If 'mini.nvim' is put in the "deps/start" part of the package (as it is in example 'init.lua'), then it will (unless done some tricky startup stuff) automatically:

On initial install, using cmdheight=0, without a notification plugin, there is no feedback at all. Using 80 plugins, it takes quite some time for the screen to become active, allowing for :messages to check the installation.

Unfortunately, there is not much to be done here, as default vim.notify() uses messages in command line to show notifications. That is why example 'init.lua' loads 'mini.notify' as early as possible.

@abeldekat
Copy link

Hello @echasnovski,

Yes, *_install will be called only on initial install to set something up only one time (like system dependencies, etc.) so as to not do this on every checkout/update. It does look like a bit of an overkill, as most (all?) others plugin manager have only single hooks ("build", "run", "do", etc.) which is called both on initial install and checkout/update. It might have been an over-engineering oversight, but it is what it is now.

I was mainly confused because of the naming, wondering if *_checkout also runs on an initial install, as an initial install does a checkout as well. Would it be an idea to mention that on an initial install the *_checkout hooks don't run?

... Be recognized as part of 'mini.deps' session. Meaning it can be updated and won't be deleted during :DepsClean.

Thanks! My bad. I somehow skipped that line when reading the docs.

Your suggestion, using add({...}, { bang = true }) made me change the default loading mechanism in my config from git submodules to mini.deps. It's perfect.

My config runs mini.deps a tiny bit faster than lazy.nvim.
However, an initial install is much slower. Lazy.nvim, from start to finish: 30 seconds. Mini.deps: 100 seconds. The installation process does run very smooth and steady.

As lazy.nvim has a dedicated spec phase, it can gather all plugins to be fetched and parallelize the git clone. Mini.deps processes per add(<someplugin>), I assume.
Perhaps a similar mechanism is possible when utilizing mini-deps-snap. If present, and there are no plugins in pack/deps, use the information to parallelize an initial git clone.

A DepsUpdate runs fine, I assume because all plugins are known to mini.deps and git operations can be parallelized.

@echasnovski
Copy link
Owner Author

I was mainly confused because of the naming, wondering if *_checkout also runs on an initial install, as an initial install does a checkout as well. Would it be an idea to mention that on an initial install the *_checkout hooks don't run?

Yeah, agree. I'll think of something.

My config runs mini.deps a tiny bit faster than lazy.nvim. However, an initial install is much slower. Lazy.nvim, from start to finish: 30 seconds. Mini.deps: 100 seconds. The installation process does run very smooth and steady.

As lazy.nvim has a dedicated spec phase, it can gather all plugins to be fetched and parallelize the git clone. Mini.deps processes per add(<someplugin>), I assume. Perhaps a similar mechanism is possible when utilizing mini-deps-snap. If present, and there are no plugins in pack/deps, use the information to parallelize an initial git clone.

A DepsUpdate runs fine, I assume because all plugins are known to mini.deps and git operations can be parallelized.

Yes, this was the big question for me when designing the whole module. I ended up making add() the way it is now for two reasons:

  • After it is called, the target plugin should be fully available together with its dependencies.
  • It should accept only single target. This makes more closely related to the built-in :packadd which is used under the hood.

This results into each plugin to be installed in sequence, not in parallel. But since it is the issue only on the first run, I think this is OK.

To make add() install in parallel, it would need to also accept list of plugin specifications. This introduces (at least) the following problems:

  • More complex logic in code and add() documentation.
  • Ideally would mean that there should be a new {pre,post}_load hooks, as loading runs in parallel.
  • Hard to force an explicit loading order (if needed) without putting some plugins to depends, which might be confusing in some cases.

So TL;DR: yes, add() works like this by design.

@abeldekat
Copy link

I think the add function is designed very well, adding to the overall simplicity of the plugin.

To make add() install in parallel, it would need to also accept list of plugin specifications. This introduces (at least) the following problems:

The idea I propose would not involve a change to add though!

Disclaimer: I scanned the code, I do not have in-depth knowledge.
Also, please consider the following as just an idea, not a request.

The core concept would be: If mini.deps knows all the plugins(as is the case on DepsUpdate), a git clone across all plugins can be very fast.

  • On start, somehow detect that an initial install will take place. An autocommand perhaps, noticing the absence of plugins in pack/deps
  • Notice that a mini-deps-snap is present
  • Read the file(as in DepsSnapLoad?), now all plugins and their versions are known
  • Git clone in parallel. Just clone, no hooks necessary. The user's config is irrelevant in this phase.
  • Proceed as usual with the first add, now or later the user has configured.

An additional benefit: Mini.deps would have a reproducible install out of the box, similar to git submodules. Currently, when installing on another machine, the latest versions of plugins are obtained. The user can perform a DepsSnapLoad. That's an extra step which is easy to forget.

@echasnovski
Copy link
Owner Author

The core concept would be: If mini.deps knows all the plugins(as is the case on DepsUpdate), a git clone across all plugins can be very fast.
...

Reading snapshot for that purpose is an interesting idea which did not occur to me initially.

However, right now it contradicts the design of snapshots themselves (which was also the target of many hours of contemplating): snapshots are there mostly for tracking per-plugin information and not as a mandatory reproduction steps to follow. That is, users are encouraged to always checkout whatever they set in checkout while occasionally making snapshot to keep VCS compatible track of the latest versions that worked as expected.

This is concurred with the fact that :DepsSnapLoad only checks out only for plugins already present session. In other words, 'init.lua' is meant as the source of truth for session and plugin state, not snapshot.

@abeldekat
Copy link

I hope you'll allow me an extra comment...)

Firstly:

But since it is the issue only on the first run, I think this is OK.

Indeed, this is not a major issue. I agree.

I used DepsSnapLoad in my example, not knowing exactly what the command does. My apologies. I thought mini-deps-snap and DepsSnapLoad to be similar in nature as lazy-lock.json and lazy restore.

I imagined the usage of the snap file in this particular case as an optional optimization, after which the config of the user would kick in with precedence.

I do understand that it's a matter of workflow. I always perform a DepsSnapSave immediately after updating. If everything works as expected, I commit the changes to mini-deps-snap. I very rarely need to add information to the checkout property. So, in my case, a reproducible initial install would be nice to have.

@echasnovski
Copy link
Owner Author

I always perform a DepsSnapSave immediately after updating. If everything works as expected, I commit the changes to mini-deps-snap. I very rarely need to add information to the checkout property. So, in my case, a reproducible initial install would be nice to have.

Yeah, me too. But not everybody has smooth experience after updating plugins, so having an information about previously working setup is useful for rollbacks.

@abeldekat
Copy link

Good morning @echasnovski,

I am aware that more complicated lazy loading and a reproducible first install is out of scope for mini.deps. Still, I hope you are interested in the remarks below.

Unfortunately, the add("someplugin", {bang=true}) solution, to register plugins loaded on key or event, can have unforeseen side effects.
For example, plugin vim-slime has an ftplugin folder. This can cause problems in the following scenario:

  • nvim
  • :e hello.py

File ftplugin.python.slime.vim is loaded. However, the vim-slime plugin is not loaded, and an error occurs.
Loading vim-slime optionally is not that important. My main use case is to load clusters of plugins on key, most notably the dap and test clusters.

So, I decided to change the approach and administer optional plugins in my own code. It was suprisingly easy, thanks to the feedback you provided in earlier conversations.

The idea: Patch some of the methods of mini.deps, using the decorator pattern.

Expanding on that same idea, I realized that a reproducible first install is also easily achievable. To me, this is an important feature, as I always check the commit message of each update. As a consequence, I only want updates when I explicitly update, not on a clean install.

The relevant parts in the config:

I noticed the updates to the readme. Thanks!

@echasnovski
Copy link
Owner Author

I am glad that you found a solution that works for you! And indeed all these look a bit complicated for the overall 'mini.deps' design, but they are necessary to accommodate all types of lazy loading (one more argument to not supporting it out of the box). My honest suggestion would be not not do it and instead use later() as sourcing of 'plugin/' directory should not take that much time.

I am thinking about the idea of adding some kind of ensure_snap_installed option which would install plugins from snapshot during setup() (enforcing reproducible setup with parallel install), but that might happen only after the 0.12.0 release.

@abeldekat
Copy link

Thanks @echasnovski!

My honest suggestion would be not not do it and instead use later() as sourcing of 'plugin/' directory should not take that much time.

It's not a matter of time though when using the later() mechanism: The additional time is not that relevant for a fast initial display of the code.

It's a matter of a slim neovim environment, having control over the amount of code active in a neovim session. I realize that this is very much up for debate...)

one more argument to not supporting it out of the box...

I think you are right. I would say that it's very nice that motivated people still can achieve the more complicated lazy loading behavior. Thanks for the plugin! I hope it gains traction.

@abeldekat
Copy link

Would it be possible to add a warning line at the top of the update buffer when there are plugins containing breaking changes?

@echasnovski
Copy link
Owner Author

Would it be possible to add a warning line at the top of the update buffer when there are plugins containing breaking changes?

That would require special parsing of commit log for when a breaking change occur. The fact that log can contain commits that are reverted (instead of added) makes it harder. So the answer is "Probably no".

I am thinking about adding special syntax highlighting for conventional commit style of commit message. Not sure if this is easily and robustly done, though.

@echasnovski
Copy link
Owner Author

With the release of 0.12.0, 'mini.deps' is now out of beta-testing. Thanks to brave early adopters who tried/switched to a fresh new plugin manger! I really appreciate it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants