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

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

Merged
merged 1 commit into from
Jul 6, 2024

Conversation

mehalter
Copy link
Contributor

@mehalter mehalter commented Jul 6, 2024

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.

@github-actions github-actions bot requested a review from stevearc July 6, 2024 13:11
@mehalter mehalter force-pushed the icon_provider_fix branch 2 times, most recently from 9b5896b to 6472e30 Compare July 6, 2024 13:18
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.
@mehalter mehalter force-pushed the icon_provider_fix branch from 6472e30 to 678630b Compare July 6, 2024 13:27
@echasnovski
Copy link

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
Copy link
Contributor Author

mehalter commented Jul 6, 2024

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
Copy link

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
Copy link
Contributor Author

mehalter commented Jul 6, 2024

Yeah this is the case @echasnovski

@stevearc stevearc merged commit d5e5657 into stevearc:master Jul 6, 2024
9 checks passed
@mehalter mehalter deleted the icon_provider_fix branch July 6, 2024 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants