mkNeovimPlugin: remove hasConfigAttrs

[GaetanLepage]

Get rid of the hasConfigAttrs which doesn't seem necessary.

Please, tell me if I missed the point and that it is (or will be) actually useful.

[khaneliman]

Makes sense to me, but would like @traxys and @MattSturgeon input.

[GaetanLepage]

For reference, it has been introduced in #1876.

[traxys]

This makes rustaceanvim a very "strange" plugin, as you can specify things in luaConfig, but they won't be written anywhere as it has configLocation = null.

I think we should either merge configLocation and hasConfigAttrs (i.e hasConfigAttrs == false is equivalent to configLocation == null).

This also means that if we want to delete them we need to delete both of them.
We may need to refactor a bit rustaceanvim to correctly insert the luaConfig to the right place in the init.lua file

[khaneliman]

This makes rustaceanvim a very "strange" plugin, as you can specify things in luaConfig, but they won't be written anywhere as it has configLocation = null.

I think we should either merge configLocation and hasConfigAttrs (i.e hasConfigAttrs == false is equivalent to configLocation == null).

This also means that if we want to delete them we need to delete both of them. We may need to refactor a bit rustaceanvim to correctly insert the luaConfig to the right place in the init.lua file

Rustaceanvim just uses the global options for its configuration, now. We just pass its settings along to our globals module https://github.com/mrcjkb/rustaceanvim?tab=readme-ov-file#gear-advanced-configuration

[traxys]

To be consistent with the other plugins we should in some way handle the fact that rustaceanvim is inserted in the globals table. Maybe we should add the preSection just before the globals and the post section after?

I'm not really sure what luaConfig means for this plugin

[khaneliman]

To be consistent with the other plugins we should in some way handle the fact that rustaceanvim is inserted in the globals table. Maybe we should add the preSection just before the globals and the post section after?

I'm not really sure what luaConfig means for this plugin

Yeah I see what you mean. I think that makes sense for a pre and post. But, I agree it's confusing for this type of plugin that basically configures like a vim plugin.

[GaetanLepage]

To be consistent with the other plugins we should in some way handle the fact that rustaceanvim is inserted in the globals table. Maybe we should add the preSection just before the globals and the post section after?

I'm not really sure what luaConfig means for this plugin

I am not sure to follow there.
Plugins, in their config, will set other options. Most of the time, it will be appending text to extraConfigLua (through luaConfig.content). But it can also (and not exclusively) be setting global variables (using config.globals) or setting any other config option.
For instance, the vim plugins using mkVimPlugin often don't add any lua code, but they set globals.
For me, there is no issue if a neovim plugin does or does not set some lua code.

What I mean is that I don't get why rustaceanvim is a special case. It's just a plugin that doesn't generate lua. That's fine no ?

[GaetanLepage]

Do we need to handle globals setting for rustaceanvim differently than what we do for vim plugins (i.e. directly leveraging globals.foo = value) ?

[GaetanLepage]

I have pushed another commit that now assumes that configLocation is never null. If someone deliberately sets it to null, it would error out.
It looks like our entire test suite evaluates to the same result as before.

Maybe we could argue for keeping those abstractions, but as it appears to me, they don't seem very useful at this time.

[traxys]

I think that we should maybe not use mkNeovimPlugin for rustaceanvim, as it is a plugin that's too special. It required adding both hasConfigAttrs & configLocation

[GaetanLepage]

It required adding both hasConfigAttrs & configLocation

Are you sure ? According to me, its behavior has not been affected by this PR. Maybe I'm missing something.

[traxys]

You can search for other occurences of configLocation

[GaetanLepage]

You can search for other occurences of configLocation

I did, and it was only present in rustaceanvim (setting it to null which didn't have any effect as callSetup was false) and in neovim-plugin.nix itself.

[traxys]

(Summary of an offline discussion)
With simply this PR there is a strange interaction with the luaConfig.{pre,post} variables, as they would not be ordered correctly for rustaceanvim.
Before removing the hasConfigAttrs (which we should do), I'd suggest we need to find a way to handle this. I have a good idea for it, I'll try to make a PR soon

[MattSturgeon]

Plugins, in their config, will set other options. Most of the time, it will be appending text to extraConfigLua (through luaConfig.content). But it can also (and not exclusively) be setting global variables (using config.globals) or setting any other config option.

I may be a bit late to the discussion and have missed some context, but the above quote seems to be conflating lua config and nix module config.

To be explicit: luaConfig.* cannot set module-config values. It can only be used for adding lines of lua to init.lua.

One of the limitations of using the module system to configure globals (and similar) is that we have no control over where the actual lua config ends up. Therefore plugins that use config.globals do not currently makes sense with luaConfig's pre/init/post system.

Most of the time we avoid this issue, because mkVimPlugin does not have a luaConfig option. However it does highlight a potential design flaw...

Perhaps in the short term it is ok to simply say "anything using mkNeovimPlugin should have some lua-based setup-style function, which makes sense to call via luaConfig" ? If so, then removing hasConfigAttrs is fine.

[GaetanLepage]

I may be a bit late to the discussion and have missed some context, but the above quote seems to be conflating lua config and nix module config.

To be explicit: luaConfig.* cannot set module-config values. It can only be used for adding lines of lua to init.lua.

One of the limitations of using the module system to configure globals (and similar) is that we have no control over where the actual lua config ends up. Therefore plugins that use config.globals do not currently makes sense with luaConfig's pre/init/post system.

Most of the time we avoid this issue, because mkVimPlugin does not have a luaConfig option. However it does highlight a potential design flaw...

Perhaps in the short term it is ok to simply say "anything using mkNeovimPlugin should have some lua-based setup-style function, which makes sense to call via luaConfig" ? If so, then removing hasConfigAttrs is fine.

We are on the same page on the understanding of the current situation.