lib/modules: init mkBlinkPluginModule

[MattSturgeon]

Initial proof of concept for how we could implement per-plugin auto-integration of blink completion providers.

So far I've drafted out some functions to produce the relevant option/modules, but I have not applied this to any plugins yet or tested it out. Posting this now for early design feedback and to discuss the concept in general.

cc @khaneliman

[MattSturgeon]

I guess we could streamline this a little, by having a blink argument passed to mkNeovimPlugin.

When present, mkNeovimPlugin could pass the argument to mkBlinkPluginModule, appending some basics like loc.

[khaneliman]

Should we just use the corresponding arguments above ?

[MattSturgeon]

Not sure what you're asking. These are probably badly named, but they're intended to be the default values for the corresponding module options.

[khaneliman]

We have enableDefault, enabldeCmdline, and enableFiletypes already in this argument list. I'm asking why they are special blink versions for the same name

[MattSturgeon]

Ah.

Because it felt unlikely that enabling a plugin as a nvim-cmp source by default should also enable it as a blink source by default. I was thinking that a plugin maintainer should manually make that decision.

We could have these defaults to the nvim-cmp equivalent, but I think it should still be possible to set these separately?

[khaneliman]

Just checking branch out to test stuff out:

… while evaluating the option `plugins.blink-cmp.settings.sources.providers.copilot.async':

       (stack trace truncated; use '--show-trace' to show the full, detailed trace)

       error: The option `plugins.blink-cmp.settings.sources.providers.copilot.async` is defined both null and not null, in `/nix/store/k3vjx39q6ksdh9m1yq9n9ikscd5d6387-source/plugins/by-name/blink-copilot' and `/nix/store/v309l0bfp6jis58v9v6a6b8rica9c72z-source/modules/nixvim/plugins/blink'.

[MattSturgeon]

Do we need an all-sources test for blink.cmp too? Unless that is covered by each plugin's own tests?

[khaneliman]

Looks good for generating config same as before. Going to try using the new config style.

Trying to enable blink-copilot configuration from it's own settings doesn't seem to generate the corresponding provider config. I see it's inserting the value into the defaults list, but it's not in the provider's attributes. Assuming it's because we're aliasing the entire existing provider config and not at a more granular level. ( I dont know if we can do anything about that, really.. :S )

EDIT: Nvm, removed all my sources config and it only generates the default sources.

[MattSturgeon]

I'm really struggling to have a warning/assertion for when the new & old systems are used in tandem.

Managed to resolve the inf-rec, by unconditionally defining for all plugins. Conditionals can still be used within the definitions though.

Now using the old+new impl together produces a sane (tested) warning.

I've also cleaned up a few more bits and fixed some tests that weren't enabling blink.

[MattSturgeon]

Trying to enable blink-copilot configuration from it's own settings doesn't seem to generate the corresponding provider config. I see it's inserting the value into the defaults list, but it's not in the provider's attributes. Assuming it's because we're aliasing the entire existing provider config and not at a more granular level. ( I dont know if we can do anything about that, really.. :S )

EDIT: Nvm, removed all my sources config and it only generates the default sources.

Reproduced, fixed, and added a test case.

Turned out that mkAliasDefinitions doesn't pick up on definitions that were already defined within a submodule-type, because those definitions don't show up in the option's definitions. Essentially such definitions belong to the type rather than the option.

[MattSturgeon]

I've included deprecating and removing autoEnableSources in this PR.

I spent some time looking for a way to gracefully transition with a warning rather than an error, however the problem is fundamentally circular and cannot be solved without disabling/removing one strategy over the other.

One concern I still have with this PR is how "auto enable" should be handled when the user wants sources to be raw-lua. Maybe instead of plugins directly adding themselves to the settings options, we should introduce an intermediate internal option and a global toggle? Such an approach could start getting cumbersome quickly, though. Perhaps its fine for users to simply disable plugins.*.cmp.enable when they want to use raw-lua?

[khaneliman]

This doesn't work the way we need it to to support blink's configuration.

      blink-copilot = {
        enable = true;

        # Need these outside of  the provider's opts
        # But don't exist in the module
        # async = true;
        # score_offset = 100;

        # Freeform accepts, but goes into `opts` instead of `providers.${cfg.key}` top level
        settings = {
          async = true;
          score_offset = 100;
        };
      };

Generates an invalid config:

copilot = {
    module = "blink-copilot",
    name = "copilot",
    opts = { async = true, score_offset = 100 },
}

I think we'd want to change it to support settings going to the top level and needing to pass opts in the settings attribute set.
https://cmp.saghen.dev/configuration/sources.html#provider-options

[khaneliman]

Yeah... so this looks like we should inherit pluginOpts.settings for the known top level options and then opts inherits from pluginOpts.settings.opts?

[MattSturgeon]

plugins.*.settings has always represented the setup function's opts, so I've attempted to be consistent with that here.

The only difference is that blink invokes the setup function for us. But that's an implementation detail that is encapsulated from users in both normal and blink plugins...

Provider settings can still be configured directly, via plugins.*.blink.settings.

It was intentional to keep these separate, but I can see how it could be confusing if you take a different perspective to me.

🤔

[khaneliman]

plugins.*.settings has always represented the setup function's opts, so I've attempted to be consistent with that here.

The only difference is that blink invokes the setup function for us. But that's an implementation detail that is encapsulated from users in both normal and blink plugins...

Provider settings can still be configured directly, via plugins.*.blink.settings.

Doesn't make this feel like a gain UX wise if they need to split their provider configuration between 2 modules, though. Personally, I'd just keep my configuration through blink-cmp so I wouldn't have to jump back and forth between 2 attribute sets to see what i'm doing. It is more niche to actually pass opts to the provider than to declare the top level provider options.

It was intentional to keep these separate, but I can see how it could be confusing if you take a different perspective to me.

🤔

I'd perfer providing the top level options to each blink plugin if we want to keep settings forwarded specifically to opts. So someone can choose to configure their provider from each source plulgin module itself.

[MattSturgeon]

I've noticed some plugins that bundle nvim-cmp sources have their own settings options for enabling the source.

e.g. codeium-nvim

In these cases should we not declare our plugins.*.cmp options? Or handle them somehow differently?

I'm guessing we'd still want blink.compat for most of these sources, too...

[MattSturgeon]

howevew

Suggested change

	The ${builtins.toJSON pluginCfg.cmp.name} nvim-cmp source has been defined via `${lib.showOption cmpLoc}`, howevew `${pluginOpt.enable}` is not enabled.
	The ${builtins.toJSON pluginCfg.cmp.name} nvim-cmp source has been defined via `${lib.showOption cmpLoc}`, however `${pluginOpt.enable}` is not enabled.

---

Also, I wonder if we can improve this warning by saying the actual settings option(s) where the source was enabled? Instead of just using cmpLoc.

This should be possible to work out from the sourceDefs attrset.