-
Notifications
You must be signed in to change notification settings - Fork 52
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
Move from after/plugin to plugin and follow plugin best pratices #61
base: main
Are you sure you want to change the base?
Conversation
Don't think adding global variables is the desired solution. |
I disagree, this is regarded as the best practice to allow users to disable your plugin from loading, and or for it not to redo it's work (which might e.g create duplicate autocmd). Source with respect to regarded: |
It should be achievable without global variables though. |
In lua, we can use - if vim.g.loaded_cmp_nvim_lsp then
+ if package.loaded['cmp_nvim_lsp'] then
return
end
- vim.g.loaded_cmp_nvim_lsp = true
require("cmp_nvim_lsp").setup() |
I think global |
Common from vim as there was no other way. @fitrh solution is way cleaner. |
I prefer @fitrh's method. |
While I understand the point as to why you would prefer the latter method in a Lua package. I might offer one last disadvantage, that is that we're not compatible with the way you can disable builtin plugins in Neovim using: local disabled_builtin_plugins = {
"netrw",
-- ...
}
for _, plugin in ipairs(disabled_builtin_plugins) do
vim.g["loaded_" .. plugin] = true
end If regardless of that we still want this change I'll make the adjustment. |
Done |
I am not convinced If I do vim.g.loaded_pluginname has been a standard for like 20 years for a reason. I would suggest revising this change to use vim.g personally. I feel using package.loaded violates the single responsibility principle thus causing possible issues like the above, and in addition vim.g. has better compatibility with vimscript |
Resolves #60