stevearc / oil.nvim

Neovim file explorer: edit your filesystem like a buffer
MIT License
3.5k stars 99 forks source link

fix: correctly check if `mini.icons` is actually setup #441

Closed mehalter closed 1 month ago

mehalter commented 1 month ago

Some users may be using the full mini.nvim suite where pcall(require, "mini.icons") will return true but doesn't mean the user has actually set it up. This verifies that it is set up by checking for the existence of _G.MiniIcons.

This leaves the pcall just so (1) we load the plugin if it is lazy loaded by the user and (2) we get LSP completion/validation with that type as well.

echasnovski commented 1 month ago

Side note: I'd personally just rely on _G.MiniIcons without require('mini.icons') as the general design is that the first one is created only as a side effect of using require('mini.icons').setup() and is equal to require('mini.icons'). This is what I use, for example.

So I'd guess that LSP completion/validation is the sole benefit here.

mehalter commented 1 month ago

The code you shared doesn't actually necessarily work if the plugin is lazy loaded. I understand it is your guidance to not lazy load the plugin, but that doesn't stop people. The check if mini.icons is setup in this PR is solely relying on _G.MiniIcons so it will only validate if the plugin is setup like you said. But it does do that pcall before checking just to make sure the plugin is loaded.

LazyVim for instance fully lazy loads mini.icons until the module is called. Out of the box this doesn't make a huge difference because of the statusline and tabline. But if a user took that distribution and removed the UI elements due to their personal choice it could leave mini.icons unloaded past startup.

This would make sure to handle that case with little to no performance loss while also respecting the guidelines laid out by mini.icons to check for it being used with _G.MiniIcons

echasnovski commented 1 month ago

Hm... This will only make sense if pcall(require, "mini.icons") would trigger require('mini.icons').setup() call from user config. Is this the case with 'lazy.nvim'?

mehalter commented 1 month ago

Yeah this is the case @echasnovski