nvim-neorocks / lz.n

šŸ¦„ A dead simple lazy-loading Lua library for Neovim plugins.
GNU General Public License v2.0
85 stars 7 forks source link

Merge import spec and non-import spec #33

Closed austinliuigi closed 2 months ago

austinliuigi commented 2 months ago

When a spec is normalized here, if it is included as both a single_plugin_spec and in the import_spec, then whichever appears first in the list will be overridden by the other.

For instance, rocks-lazy.nvim inserts the import at the very end of the spec list, so if a plugin has specs provided in lua via import, then it would override any specs provided in rocks.toml. Additionally, since rocks.nvm injects its before hook along with the toml specs, that would also get overridden, which causes the plugin to not get configured if the user is using rocks-config.nvim.

This seems like it needs to be handled by lz.n instead of rocks-lazy.nvim because lz.n is the one doing the importing, so rocks-lazy.nvim would have no idea about which plugins are configured using lua. My suggestion would be to specify the priority between imported specs and non-imported specs and handle it appropriately with, e.g. a vim.tbl_deep_extend() here.

Edit: After looking again at rocks-lazy.nvim, I noticed the README says it will be merged, but that doesn't seem to be the case.

mrcjkb commented 2 months ago

I'm not sure I understand the issue.

rocks-lazy.nvim has a test case to verify that you can use both rocks.toml and lua configs.

I've corrected the tip box in the readme. It doesn't merge configs, it just extends them (assuming there are no duplicates). Is there a use case for having duplicate specs? Merging duplicates would override rocks-config hooks, too.

austinliuigi commented 2 months ago

Oh I see. I misunderstood the tip box as saying you can use both rocks.toml entries and lua to configure specs for the same plugin. So is it expected for the config_hook that rocks.nvim injects into rocks.toml entries to not run for plugins that have lua configured specs?

austinliuigi commented 2 months ago

Merging duplicates would override rocks-config hooks, too.

Not if the user doesn't provide an explicit before hook of their own

mrcjkb commented 2 months ago

So is it expected for the config_hook that rocks.nvim injects into rocks.toml entries to not run for plugins that have lua configured specs?

Yes, that's a current limitation. Lua configured specs are just meant to be used if you need flexibility that you can't get with toml (like conditionally defining events). But they don't integrate with rocks-config.nvim.

Not if the user doesn't provide an explicit before hook of their own

That's a fair point. We could potentially use vim.tbl_deep_extend("error", ...) to prevent the hook from accidentally being overridden (and if users want to override it, they'd have to use only Lua specs).

I'll have to see how such a feature would increase complexity and then decide if it's worth it.

austinliuigi commented 2 months ago

Ok got it. I think it should be mentioned in the rocks-lazy.nvim README that lua configured specs aren't integrated with rocks-config.nvm because that caused me quite a bit of confusion.

That's a fair point. We could potentially use vim.tbl_deep_extend("error", ...) to prevent the hook from accidentally being overridden (and if users want to override it, they'd have to use only Lua specs).

I'm not quite sure I understand. Are you using this to merge the rocks.toml spec with the lua spec for a plugin? If so, then I don't think "error" would work to allow overriding.

mrcjkb commented 2 months ago

I think it should be mentioned in the rocks-lazy.nvim README

Updated.

Regarding merging specs: The reason this library doesn't do so is that with a simple vim.tbl_deep_extend, the behaviour may not be deterministic. Implementing proper merging, as lazy.nvim does, requires complex (and potentially buggy + definitely difficult to maintain) state management (combined with filesystem IO due to the import specs). I do not believe that such a feature warrants the added complexity, which is why I had decided that it is out of scope for lz.n.