-
Notifications
You must be signed in to change notification settings - Fork 35
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
Configure plugin via global variable #90
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! Had a few comments.
I'm not against adding the global variable configuration option, but I'm wondering why people would prefer this instead of the setup
function approach. As far as I can tell, a setup
function is more clear and easier to understand:
- There are type hints, providing completion and some degree of validation:
- It's quite clear when the config will be applied: when you call the
setup
function. With a global variable it's up to the plugin internals to read the config at some point. We're reading it in thets_context_commentstring.config
module, but when exactly is that evaluated? And what if you set your global variables after that gets evaluated? - The
setup
function is quite standard, following the format ofrequire('<plugin name>').setup({ ... })
whereas the global variable can have whatever name the plugin author decides to give it. - Lazy.nvim can set up the plugin with
opts
, calling thesetup
function automatically.
So, I'm just wondering why people would prefer to use the global variable? I suppose it's more similar to how Vim plugins have classically been configured, but I don't really see any advantages over a setup
function. Maybe you can shed some light on this?
|
||
---@return ts_context_commentstring.CommentaryConfig | table | ||
function M.commentary_integration() | ||
return config.commentary_integration or {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels like this could just return nil
as an empty table doesn't really signify that the value is "missing"
|
||
config = vim.tbl_deep_extend('force', {}, default_config, overrides) | ||
config.languages = vim.tbl_deep_extend('force', config.languages, config.config) | ||
configured_languages = vim.tbl_keys(config.languages) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keeping track of the configured languages like this seems a bit unnecessary to me. We could just return the keys inside configured_languages()
:
function M.configured_languages()
return vim.tbl_keys(config.languages)
end
This way, if we ever have any other ways to update the config
, we won't have to keep configured_languages
up to date.
function M.get_languages_config() | ||
return vim.tbl_deep_extend('force', M.config.languages, M.config.config) | ||
---@return table<string> | ||
function M.configured_languages() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer the names of these functions to be more descriptive as it's not clear what exactly it does. "configured languages" seems more like a variable name to me rather than a function name. I usually like to prefix such functions with "get", "get configured languages" and "get commentary integration" seem more clear to me.
What do you think?
---@cast node -nil | ||
---@cast language_tree -nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, didn't know this was the way to do type guards with the type annotations 👍
If you want to change the configuration, provide custom overrides in `vim.g.ts_context_commentstring_config` before it's loaded | ||
|
||
```lua | ||
vim.g.ts_context_commentstring_config = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if I would prefer to have the global variable to be the suggested way of configuring the plugin, for the reasons mentioned in the other comment. Will need to think about this a little bit 😄
Type hints are a single @as annotation away. The great thing about annotations is that neovim startup process doesn’t freak out if they point at something unknown, whereas setup routine breaks if the plugin is not present. Neovim's initialization process is a well documented routine - https://neovim.io/doc/user/starting.html#initialization. If you set the global variable in your local configuration, it precedes plugin loading.
It's definitely not. The name to require is decided by the plugin author. It's all about the folder and file names in Anyways, https://mrcjkb.dev/posts/2023-08-22-setup.html explains it better than I could. There's no need to call |
This change allows setting
vim.g.ts_context_commentstring_config
somewhere in, for example,~/.config/nvim/plugin/ts_commentstring_config.lua
and have the plugin just work without any need to explicitly set it up.The configuration is read when config.lua is loaded, e.g. on the first
require('ts_context_commentstring.config')
.