lib/neovim-plugin: Introduce `plugins.*.luaConfig.*` options by traxys · Pull Request #1876 · nix-community/nixvim

traxys · 2024-07-16T19:44:11Z

Resolves #1407

This commit adds a plugins.<name>.config section controlling the plugin specific configuration.

The section contains the internal init option, containing the plugin's initialization code.

It also contains the public pre and post options, that allow to add code before & after the init section

Finally, it contains the final option, being the concatenation of the three previous options.

I would have like to have a location option that allows to specify where to add the configuration option, it could be useful for rustaceanvim that needs to put the config in different places depending on plugins.lsp.enable, but that ended up in infinite recursions.
This option could also have been useful for lazy loading, as it may have enabled to add the configuration in a separate function to accomodate lazy loading.

The implementation is a bit less clean than I would have liked, as the flash plugin already defines a config option, so I had to write some code to skip adding this new option for that plugin

Old description

Those new options are scope to the plugin they help to configure. Those options should be preferred internally to using the raw extraConfigLua(Pre|Post)? option, as it allows being more flexible with how the configuration is generated.

This commit does not introduce any customization, but doing this can allow things like:

Easily splitting the plugin in another lua file, for example for a specific filetype
Passing the configuration to a lazy loading provider

This also allows users to define helper functions close to the plugins they are used into, allowing a more modular configuration

This improvement is only available to plugins using mkNeovimPlugin right now.

GaetanLepage

LGTM !

lib/neovim-plugin.nix

MattSturgeon · 2024-08-03T23:25:33Z

Can we revisit this now the CI is working again?

traxys · 2024-08-04T13:17:37Z

Yup, I thought about it a bit and I think I'll go with your suggestion, of creating a single lua block with pre|post

traxys · 2024-08-19T21:41:02Z

@MattSturgeon I re-wrote the PR and amended the description with the new implementation details!

MattSturgeon

Initial thoughts and questions, thanks for continuing to work on this!

lib/neovim-plugin.nix

plugins/colorschemes/ayu.nix

plugins/languages/rust/rustaceanvim/default.nix

Outdated

lib/neovim-plugin.nix

plugins/utils/flash.nix

plugins/utils/hydra/default.nix

lib/neovim-plugin.nix

plugins/languages/rust/rustaceanvim/default.nix

lib/neovim-plugin.nix

plugins/languages/rust/rustaceanvim/default.nix

MattSturgeon

I don't see any issues, but it might be nice to mark luaConfig.final as internal?

I also think it may be more powerful/future-proof to make final a lines type and make use of ordered definitions, see #1876 (comment).

lib/types.nix

khaneliman

Really like where this PR has gone in giving more control over lua relevant to a plugin

stasjok · 2024-09-14T06:43:05Z

Maybe wrap the config in do ... end block? Or even add an additional option like wrapInDo default true. This PR is very useful, but usually I try to write self-contained blocks of code and do block ensures that I don't pollute locals or don't override something unintentionally.

stasjok · 2024-09-14T06:45:57Z

plugins/utils/ccc.nix


-    extraConfigLua = ''
+    plugins.ccc.luaConfig.init = ''
      ccc = require('ccc')


It's not introduced by this PR but why is it not local? Hydra later too.

plugins/utils/rest.nix

traxys · 2024-09-21T21:43:04Z

Maybe wrap the config in do ... end block? Or even add an additional option like wrapInDo default true. This PR is very useful, but usually I try to write self-contained blocks of code and do block ensures that I don't pollute locals or don't override something unintentionally.

Very good idea, though I think I'll implement that in a follow-up PR (meanwhile you can simulate it by adding a do in luaConfig.pre an end in luaConfig.post)

Stale review

lib/types.nix

plugins/utils/rest.nix

khaneliman

LGTM I think this will be a very useful change

MattSturgeon

Looks good overall! I just wonder if the descriptions could be clarified?

MattSturgeon · 2024-09-22T13:43:51Z

lib/types.nix

+        pre = lib.mkOption {
+          type = types.lines;
+          default = "";
+          description = "Configuration before the initialization of the plugin";


I think this description could clarify that pre is actually merged into init with a "before" merge-order priority

Seems like an implementation level detail that would cloud the purpose of the option.

I don't think the fact that pre and post are merged into init is purely an implementation detail:

End users will be exposed to this detail when reading the final init value

mkOrder is a user-facing utility

how mkOrder interacts with pre & post could be important to some users

I think the implementation is fine, so long as users aren't surprised by it.

* End users will be exposed to this detail when _reading_ the final `init` value

True, this makes sense to me as a reason to include some mention, then.

MattSturgeon · 2024-09-22T13:44:04Z

lib/types.nix

+        post = lib.mkOption {
+          type = types.lines;
+          default = "";
+          description = "Configuration after the initialization of the plugin";


MattSturgeon · 2024-09-22T13:46:14Z

lib/types.nix

+        init = lib.mkOption {
+          type = types.lines;
+          default = "";
+          description = "Initialization code (between pre & post)";


Same;

"between" pre&post is actually a little misleading. Anyone that reads init will see it as including pre&post.

Anyone who uses mkOrder on init may also be confused when pre&post get merged in-between their ordered definitions.

Hmm... this makes me not like tying them to the same mkOrder merge if we can have this interaction. It should be set that pre comes before init and post comes after.

Yeah I think I should change the name, to something like config or content

MattSturgeon · 2024-09-22T13:50:15Z

lib/types.nix

+        (lib.mkBefore config.pre)
+        (lib.mkAfter config.post)


I wonder if it better to use non-standard merge priority numbers?

E.g. what would we expect to happen when someone uses:

pre = "foo"; init = mkBefore "bar";

Currently they would both be merged at priority 500, however we can't know just from reading the definition if we'll get "foo\nbar" or "bar\nfoo".

I think users would expect pre to be merged earlier than mkBefore and post to be merged after mkAfter?

Definitely a niche case, but makes sense to at least prevent default priority overrides from messing with ordering.

MattSturgeon · 2024-09-22T15:57:35Z

lib/types.nix

+          default = "";
+          description = ''
+            Lua code inserted at the start of the plugin's configuration.
+            This is simply sugar around `lib.nixvim.utils.mkBeforeSection content`


Suggested change

This is simply sugar around `lib.nixvim.utils.mkBeforeSection content`

This is the same as using `lib.nixvim.utils.mkBeforeSection` when defining `content`.

Or maybe something like

Suggested change

This is simply sugar around `lib.nixvim.utils.mkBeforeSection content`

This is the same as `content = lib.mkOrder ${(lib.nixvim.utils.mkBeforeSection null).priority} ""`

?

I'd prefer the first one

MattSturgeon

Awesome, LGTM!

MattSturgeon · 2024-09-22T16:08:32Z

lib/types.nix

+        (lib.nixvim.utils.mkBeforeSection config.pre)
+        (lib.nixvim.utils.mkAfterSection config.post)


Strictly speaking, these should each be wrapped in mkIf (config.pre != ""), to avoid unnecessary empty lines when no pre/post is defined.

Or we could drop their default and use isDefined. Or we could keep their default and use highestPrio < 1500.

Regardless of the approach used, it will likely end up kinda messy. Therefore I don't think we need to necessarily tackle it in this PR.

traxys · 2024-09-22T16:14:57Z

@Mergifyio queue

mergify · 2024-09-22T16:15:25Z

queue

✅ The pull request has been merged automatically

Details

The pull request has been merged automatically at d2f9e01

This commit adds a `plugins.<name>.luaConfig` section controlling the plugin specific configuration. The section contains the internal `init` option, containing the plugin's initialization code. It also contains the public `pre` and `post` options, that allow to add code before & after the `init` section Finally, it contains the `final` option, being the concatenation of the three previous options.

mergify · 2024-09-22T16:19:02Z

This pull request, with head sha d2f9e011d9c6e3d765422b78039e61ddf9e69749, has been successfully merged with fast-forward by Mergify.

This pull request will be automatically closed by GitHub.

As soon as GitHub detects that the sha d2f9e011d9c6e3d765422b78039e61ddf9e69749 is part of the main branch, it will mark this pull request as merged.

It is possible for this pull request to remain open if this detection does not happen, this usually happens when a force-push is done on this branch configLuaPlugin, this means GitHub will fail to detect the merge.

traxys requested a review from MattSturgeon July 16, 2024 19:44

GaetanLepage previously approved these changes Jul 16, 2024

View reviewed changes

MattSturgeon reviewed Jul 17, 2024

View reviewed changes

lib/neovim-plugin.nix Outdated Show resolved Hide resolved

MattSturgeon reviewed Jul 17, 2024

View reviewed changes

lib/neovim-plugin.nix Outdated Show resolved Hide resolved

psfloyd mentioned this pull request Aug 8, 2024

lib/neovim-plugin: Add lazyLoad option to plugins #1875

Closed

6 tasks

traxys force-pushed the configLuaPlugin branch from 03e1061 to 785f7dc Compare August 19, 2024 21:36

MattSturgeon reviewed Aug 19, 2024

View reviewed changes

MattSturgeon changed the title ~~lib/neovim-plugin: Introduce configLua(Pre|Post)? options~~ lib/neovim-plugin: Introduce plugins.*.config.* options Aug 19, 2024

traxys force-pushed the configLuaPlugin branch 2 times, most recently from ee22c8d to 7c6e818 Compare August 20, 2024 20:20

MattSturgeon reviewed Aug 20, 2024

View reviewed changes

khaneliman reviewed Aug 20, 2024

View reviewed changes

plugins/languages/rust/rustaceanvim/default.nix Outdated Show resolved Hide resolved

traxys force-pushed the configLuaPlugin branch from 7c6e818 to ce3e294 Compare August 21, 2024 19:24

MattSturgeon mentioned this pull request Aug 22, 2024

plugins/rustaceanvim: fix checkhealth error #2049

Merged

traxys force-pushed the configLuaPlugin branch from ce3e294 to 6868413 Compare August 28, 2024 20:59

traxys changed the title ~~lib/neovim-plugin: Introduce plugins.*.config.* options~~ lib/neovim-plugin: Introduce plugins.*.luaConfig.* options Aug 28, 2024

MattSturgeon previously approved these changes Aug 29, 2024

View reviewed changes

lib/types.nix Outdated Show resolved Hide resolved

khaneliman previously approved these changes Aug 29, 2024

View reviewed changes

traxys mentioned this pull request Sep 9, 2024

[Feature] Lazy loading #421

Closed

stasjok reviewed Sep 14, 2024

View reviewed changes

khaneliman mentioned this pull request Sep 21, 2024

plugins/avante: init #2285

Merged

traxys force-pushed the configLuaPlugin branch 2 times, most recently from ca5771d to d8e1b37 Compare September 21, 2024 20:56

khaneliman reviewed Sep 21, 2024

View reviewed changes

plugins/utils/rest.nix Outdated Show resolved Hide resolved

traxys force-pushed the configLuaPlugin branch from d8e1b37 to fa2d4eb Compare September 21, 2024 21:41

traxys force-pushed the configLuaPlugin branch from fa2d4eb to a747ffe Compare September 21, 2024 21:44

traxys requested review from MattSturgeon and khaneliman September 21, 2024 22:13

khaneliman reviewed Sep 21, 2024

View reviewed changes

lib/types.nix Outdated Show resolved Hide resolved

khaneliman reviewed Sep 21, 2024

View reviewed changes

plugins/utils/rest.nix Outdated Show resolved Hide resolved

traxys force-pushed the configLuaPlugin branch from a747ffe to 3a63e2a Compare September 22, 2024 10:08

khaneliman approved these changes Sep 22, 2024

View reviewed changes

MattSturgeon reviewed Sep 22, 2024

View reviewed changes

traxys force-pushed the configLuaPlugin branch 4 times, most recently from f403a1d to f5c3b13 Compare September 22, 2024 15:54

MattSturgeon reviewed Sep 22, 2024

View reviewed changes

traxys force-pushed the configLuaPlugin branch from f5c3b13 to 6c688ec Compare September 22, 2024 16:00

MattSturgeon approved these changes Sep 22, 2024

View reviewed changes

traxys force-pushed the configLuaPlugin branch from 6c688ec to 0aa8ff6 Compare September 22, 2024 16:11

nix-infra-bot force-pushed the configLuaPlugin branch from 0aa8ff6 to d2f9e01 Compare September 22, 2024 16:15

mergify bot merged commit d2f9e01 into nix-community:main Sep 22, 2024

mergify bot temporarily deployed to github-pages September 22, 2024 16:19 Inactive

MattSturgeon mentioned this pull request Sep 22, 2024

lib/types/pluginLuaConfig: only merge pre and post when defined #2294

Merged

xav-ie mentioned this pull request Nov 30, 2024

blog/nix-lazy-neovim breuerfelix/breuerfelix.github.io#33

Open

GaetanLepage mentioned this pull request Dec 8, 2024

mkNeovimPlugin: remove hasConfigAttrs #2619

Closed

	This is simply sugar around `lib.nixvim.utils.mkBeforeSection content`
	This is the same as using `lib.nixvim.utils.mkBeforeSection` when defining `content`.

	This is simply sugar around `lib.nixvim.utils.mkBeforeSection content`
	This is the same as `content = lib.mkOrder ${(lib.nixvim.utils.mkBeforeSection null).priority} ""`

		(lib.nixvim.utils.mkBeforeSection config.pre)
		(lib.nixvim.utils.mkAfterSection config.post)

Uh oh!

Conversation

traxys commented Jul 16, 2024 • edited by khaneliman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Old description

Uh oh!

GaetanLepage left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MattSturgeon commented Aug 3, 2024

Uh oh!

traxys commented Aug 4, 2024

Uh oh!

traxys commented Aug 19, 2024

Uh oh!

MattSturgeon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MattSturgeon left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

khaneliman left a comment

Choose a reason for hiding this comment

Uh oh!

stasjok commented Sep 14, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

traxys commented Sep 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

khaneliman left a comment

Choose a reason for hiding this comment

Uh oh!

MattSturgeon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MattSturgeon Sep 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MattSturgeon Sep 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MattSturgeon Sep 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

traxys commented Jul 16, 2024 •

edited by khaneliman

Loading

MattSturgeon left a comment •

edited

Loading

traxys commented Sep 21, 2024 •

edited

Loading

MattSturgeon Sep 22, 2024 •

edited

Loading

MattSturgeon Sep 22, 2024 •

edited

Loading

MattSturgeon Sep 22, 2024 •

edited

Loading

mergify bot commented Sep 22, 2024 •

edited

Loading