echasnovski / mini.nvim

Library of 40+ independent Lua modules improving overall Neovim (version 0.8 and higher) experience with minimal effort
MIT License
5.23k stars 189 forks source link

Beta-testing 'mini.deps' #689

Closed echasnovski closed 8 months ago

echasnovski commented 8 months ago

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):

Thanks!

AlexKurisu commented 8 months ago

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

echasnovski commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 first one looks more appropriate, but the second one looks more doable.

wroyca commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

I use neovim nightly btw

echasnovski commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

@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 commented 8 months ago

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 commented 8 months ago

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

Code ```lua -- 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 commented 8 months ago

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

echasnovski commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

@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 commented 8 months ago

@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 commented 8 months ago

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>)

echasnovski commented 8 months ago

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>)

Yeah, that sounds reasonable.

I thought about adding some kind of table with plugin spec to hooks, but figured out it would be redundant as all the information is available to the user in the specification. But as path is more or less computed, it might make be reasonable, yes. I'll look into it.

echasnovski commented 8 months ago

I've just pushed an update to main addressing new feedback.


@hosaka, it should now work on 2.34.1 by dropping a non-compatible option entirely. At least, I hope (no actual testing was done on it, just quick look at which other options might be too new).

Also took an opportunity and used the version computing logic to make sure that Git executable is available before performing any Git operation.


@AlexKurisu, hooks now take table with certain data as argument. Decided to include only fields that are usually inferred. Did not include checkout and monitor because they are not yet inferred during pre_* hooks (but might reconsider for a reasonably important use case).

eckig commented 8 months ago

I ported my config to mini.deps and it works fine 👍

The only minor annoyance: :DepsClean lists mini.nvim as to be removed and will even delete it..

echasnovski commented 8 months ago

I ported my config to mini.deps and it works fine 👍

That's really helpful to know during beta-testing, thanks for sharing!

The only minor annoyance: :DepsClean lists mini.nvim as to be removed and will even delete it..

I assume you didn't put it in "start" directory as in Installation guide or example 'init.lua'? If you don't want it to be in "start", you can add add('mini.nvim') in your 'init.lua' to add it to session.

eckig commented 8 months ago

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

eglimi commented 8 months ago

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 commented 8 months ago

I also ported from lazy and all works great.

:rocket:

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

@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 commented 8 months ago

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 commented 8 months ago

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. ...

- 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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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:

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:

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

abeldekat commented 8 months ago

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.

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 commented 8 months ago

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.