search

nix-community / nixvim

Configure Neovim with Nix! [maintainer=@GaetanLepage, @traxys, @mattsturgeon, @khaneliman]
https://nix-community.github.io/nixvim
MIT License
1.59k stars 251 forks source link

Standardize Shared Packages/Plugins #2119

Open khaneliman opened 2 weeks ago

khaneliman commented 2 weeks ago

In response to https://github.com/nix-community/nixvim/pull/2112#issuecomment-2320384864 it would be good to consolidate our usages for certain extraPlugins and extraPackages that will affect other plugins. Currently, we do a lot of stuff with git and nvim-web-devicons, but there are other packages/plugins that are added as optional dependencies that have side effects on all / other plugins.

I'm not sure where this option should live and what it should look like, yet.

MattSturgeon commented 2 weeks ago

Perhaps we could do cfg.package.override (prev: { dependencies = prev.dependencies ++ [ cfg.fooPackage ]; })?

This would keep things a bit more isolated...

khaneliman commented 2 weeks ago

I don't think that will actually isolate the additional plugin for an individual plugin that has the setting. The inclusion of the dependency for lualine upstream caused side effects with other plugins because it just gets installed along with it.

stasjok commented 2 weeks ago

Actually in my comment I meant something like a global option iconsProvider. And the user can change it to nvim-web-devicons or mini.icons with the ability to change settings for those providers. Both have it's own setup() function. To emulate nvim-web-devicons api MiniIcons provides MiniIcons.mock_nvim_web_devicons() function.

But maybe they both should be just plugins with it's own enable options. Mini.nvim already have a plugin definition, but there is no option to mock nvim-web-devicons (MiniIcons.mock_nvim_web_devicons() should probably be called in extraConfigLuaPre before other plugins to emulate nvim-web-devicons (if enabled)). For now if I want to use mini.icons, I need to set iconsPackage to null for every plugin explicitly. And I need to call MiniIcons.mock_nvim_web_devicons() somewhere (ideally there should be an option for this).

Also maybe a known icon provider plugins can change some internal value when enabled. In other plugins instead of iconsPackage the value of this option can be checked. If some icon plugin is already enabled, then do nothing. If nothing is enabled, then enable some default implementation (nvim-web-devicons).

MattSturgeon commented 2 weeks ago

Thanks for your insight, that's helpful context! Looking at them as plugins in their own right, I agree we should handle them as plugins.*.enable.

Calling MiniIcons.mock_nvim_web_devicons could be an extra-option under plugins.mini; e.g. plugins.mini.mockDevIconsAPI or something. IDK if it should be on or off by default.

Enabling nvim-web-devicons could be a plugin such as plugins.devicons.enable. This especially makes sense if the plugin has a setup function with config options.

Plugins that depend (or optionally depend) on these can handle this in the same way we currently do for (e.g.) treesitter and/or telescope dependencies. I.e. recommend through documentation and/or warnings that the other plugin/option be enabled.

I think the newly introduced iconsPackage options are probably new enough that we don't need to gracefully deprecate them; we could maybe just have a mkRemovedOptionModule to print a useful assertion message if the option is defined.

khaneliman commented 1 week ago

Thanks for your insight, that's helpful context! Looking at them as plugins in their own right, I agree we should handle them as plugins.*.enable.

Calling MiniIcons.mock_nvim_web_devicons could be an extra-option under plugins.mini; e.g. plugins.mini.mockDevIconsAPI or something. IDK if it should be on or off by default.

I think it makes sense to be enabled if we detect a user configuring mini.icons so someone can easily treat either plugin as an icon provider.

Enabling nvim-web-devicons could be a plugin such as plugins.devicons.enable. This especially makes sense if the plugin has a setup function with config options.

Plugins that depend (or optionally depend) on these can handle this in the same way we currently do for (e.g.) treesitter and/or telescope dependencies. I.e. recommend through documentation and/or warnings that the other plugin/option be enabled.

I'm trying to implement the warning/assertion for this behavior and it made me question how we want to check it. There can be multiple packages that provide icons, should we have a property/list that is populated with plugins that are icon providers to check if a user has one enabled? Otherwise, our method of telescope has been to explicitly check if it's enabled, but that won't scale well I think for icon providers.

I think the newly introduced iconsPackage options are probably new enough that we don't need to gracefully deprecate them; we could maybe just have a mkRemovedOptionModule to print a useful assertion message if the option is defined.

This makes sense, I was close to just reverting it so people don't get used to it for too long. But, was hoping to just create this new behavior quick enough that we can rip it out in the new feature.