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

Move from after/plugin to plugin and follow plugin best pratices #61

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

tvercruyssen
Copy link
Contributor

@tvercruyssen tvercruyssen commented Sep 2, 2023

Resolves #60

@gegoune
Copy link

gegoune commented Sep 2, 2023

Don't think adding global variables is the desired solution.

@tvercruyssen
Copy link
Contributor Author

tvercruyssen commented Sep 2, 2023

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:
examples of a few popular plugins (by vim-awesome):

@gegoune
Copy link

gegoune commented Sep 2, 2023

It should be achievable without global variables though.

@fitrh
Copy link

fitrh commented Oct 4, 2023

In lua, we can use package.loaded[mod]

- 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()

@wookayin
Copy link
Contributor

I think global load_... variables for plugin is fine, as it's a very common convention.

@gegoune
Copy link

gegoune commented Nov 16, 2023

Common from vim as there was no other way. @fitrh solution is way cleaner.

@hrsh7th
Copy link
Owner

hrsh7th commented Dec 10, 2023

I prefer @fitrh's method.
Could you please change it like that?

@tvercruyssen
Copy link
Contributor Author

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.

@tvercruyssen
Copy link
Contributor Author

I prefer @fitrh's method. Could you please change it like that?

Done

@BirdeeHub
Copy link

BirdeeHub commented Nov 8, 2024

I am not convinced package.loaded is cleaner. Even for a pure Lua solution.

If I do require('cmp_nvim_lsp').default_capabilities() in my lsp setup called by my init.lua, this sets the package.loaded.cmp_nvim_lsp variable. init.lua gets ran before the plugin dirs are sourced. That then prevents this hook from running require('cmp_nvim_lsp').setup(), thus still requiring me to manually call setup.

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

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.

Use of after directory, antipattern.
6 participants