plugins/lz-n: init #1874

psfloyd · 2024-07-16T05:17:06Z

This is the first PR of splitting PR #1866.

This PR adds:

plugins.lz-n and its options for lazy-loading plugins using lz.n.
Tests for plugins.lz-n.

MattSturgeon

Thanks for working on this!

It's looking good, I've just left a few comments below to hopefully improve things further

plugins/pluginmanagers/lz-n.nix

psfloyd · 2024-07-17T03:04:00Z

I have made the changes suggested by the review.

A question more about GitHub ettiquette. Should I keep these changes in its own commit, or should i squash them to the original commit?

MattSturgeon

I have made the changes suggested by the review.

Thanks!

A question more about GitHub ettiquette. Should I keep these changes in its own commit, or should i squash them to the original commit?

Personally, it doesn't bother me either way.

Having separate commits can be beneficial since that way you can more easily tell what has changed, although it's still possible to diff force-pushes.

The most important thing is that it is easy to squash the changes once the PR has moved on enough that having separate commits is no longer needed.

On more complex PRs you can end up with conflicts when squashing lots of small changes that were committed in a different order.

Also, if you're not aware, git commit --fixup and git rebase -i --autosquash can be very useful.

I've focused this review on the plugins option and attempting to make it fit better with RFC42.

See this section of our contributing guide, or the RFC itself.

For some of the discussion points I'd appreciate input from the other maintainers @nix-community/nixvim

plugins/pluginmanagers/lz-n.nix

psfloyd · 2024-07-18T05:40:45Z

Thank you for your review and guidance!

I made most of the changes suggested. Some things still need input from reviewers.

plugins/pluginmanagers/lz-n.nix

psfloyd · 2024-07-21T02:41:36Z

I think I resolved most of the concerns. All the magic for setting options was removed.
All trigger options now accept rawLua.

If there is anything else I need to change, please let me know.

plugins/pluginmanagers/lz-n.nix

MattSturgeon

A few nits, but mostly just discussion points.

The main blocker now is discussing the best way to handle documenting unkeyed attrs as part of a well-defined submodule.

The PR itself is in good shape, well done! 🎉

MattSturgeon · 2024-07-26T12:24:58Z

plugins/pluginmanagers/lz-n.nix

+    submodule {
+      freeformType = attrsOf anything;
+      options = {
+        __unkeyed = mkOption {


listToUnkeyedAttrs uses __unkeyed-1:

nixvim/lib/utils.nix

Lines 15 to 17 in 47b6c48

listToUnkeyedAttrs =

list:

builtins.listToAttrs (lib.lists.imap0 (idx: lib.nameValuePair "__unkeyed-${toString idx}") list);

I guess for consistency we should too? On the other hand maybe it's better is this doesn't specify an index, to allow users to add unkeyed table entries with any index (via freeform definitions)?

I don't think we've ever had an "unkeyed" option definition before... So far all unkeyed attrs have been in freeform definitions.

I can't see a better alternative, it just feels as though we're now more publicly exposing the messy side of trying to map nix values to lua tables. And in doing so committing more solidly to our current strategy? 🫤

@traxys are you happy with this approach?

plugins/pluginmanagers/lz-n.nix

tests/test-sources/plugins/pluginmanagers/lz-n.nix

plugins/pluginmanagers/lz-n.nix

MattSturgeon

Thanks, I think this is pretty much done. There's just a couple thinks I'd like a second opinion on from another maintainer.

Other than that it's just nits below. Now the PR is pretty much done, could you try and keep the commits squashed to just the two "add maintainer" and "add plugin" commits?

Also, sorry this took a while for me to look at. If it's ready for review and you have been waiting, don't hesitate to ping one of us, since it's easy for individual PRs to get forgotten about otherwise!

plugins/pluginmanagers/lz-n.nix

MattSturgeon · 2024-08-03T21:53:01Z

plugins/pluginmanagers/lz-n.nix

+            enabled = helpers.defaultNullOpts.mkStrLuaFnOr bool true ''
+              When false, or if the function returns false, then this plugin will not be included in the spec.
+              This option corresponds to the `enabled` property of lz.n.
+            '';
+            beforeAll = helpers.mkNullOrLuaFn "Always executed before any plugins are loaded.";
+            before = helpers.mkNullOrLuaFn "Executed before this plugin is loaded.";
+            after = helpers.mkNullOrLuaFn "Executed after this plugin is loaded.";
+            load = helpers.mkNullOrLuaFn "Can be used to override the `vim.g.lz_n.load()` function for this plugin.";


@GaetanLepage @traxys are we happy to introduce new strLua options if we're considering deprecating strLua?

I guess this is fine since we haven't properly discussed it yet and made a decision?

plugins/pluginmanagers/lz-n.nix

tests/test-sources/plugins/pluginmanagers/lz-n.nix

psfloyd · 2024-08-03T23:35:46Z

Great, thanks for taking a look at it again. I applied all the changes and some other style changes suggested by GaetanLepage on #1979. I sqashed all comits as requested.

No worries about the wait, I took that time to improve my personal nixvim config ;).

Regarding the strLua options, if it is necesary I can reafactor the options with another type. I prefer to have a stable interface for #1875.

plugins/pluginmanagers/lz-n.nix

MattSturgeon · 2024-08-03T23:41:28Z

Regarding the strLua options, if it is necesary I can reafactor the options with another type. I prefer to have a stable interface for #1875.

I think it's fine. Nixvim is full of strLua, so this won't make any eventual deprecation notably worse.

I think other maintainers will take a look over the next few days 🤞

MattSturgeon · 2024-08-03T23:42:13Z

Great, thanks for taking a look at it again. I applied all the changes and some other style changes suggested by GaetanLepage on #1979. I sqashed all comits as requested.

Thanks for your persistent work & dedication on these PRs!

MattSturgeon · 2024-08-04T00:10:37Z

cannot build on 'ssh-ng://[email protected]': error: failed to start SSH connection to '[email protected]'
Failed to find a machine for remote build!
derivation: 39zgfnw2zax5d7qyr7894a99dizldi7i-plugins-utils-mark-radar.drv
required (system, features): (aarch64-linux, [])
2 available machines:
(systems, maxjobs, supportedFeatures, mandatoryFeatures)
([aarch64-linux], 80, [benchmark, big-parallel, gccarch-armv8-a, kvm, nixos-test], [])
([aarch64-darwin, x86_64-darwin], 8, [big-parallel], [])
error: a 'aarch64-linux' with features {} is required to build '/nix/store/39zgfnw2zax5d7qyr7894a99dizldi7i-plugins-utils-mark-radar.drv', but I am a 'x86_64-linux' with features {benchmark, big-parallel, kvm, nixos-test}

Seems like a buildbot issue? Just strange it's only happening on this PR. Will try rebuilding.

psfloyd · 2024-08-04T00:36:39Z

I think other maintainers will take a look over the next few days 🤞

Great!

Thanks for your persistent work & dedication on these PRs!

Thanks to all of you for maintaining this awesome project.

Seems like a buildbot issue? Just strange it's only happening on this PR. Will try rebuilding.

I saw the error, but since it was not in the lz-n tests I didn't know how to troubleshoot it.

GaetanLepage

Minor nit, otherwise good to me !

plugins/pluginmanagers/lz-n.nix

MattSturgeon

LGTM, are you happy @GaetanLepage?

GaetanLepage

Yes, let's go !

GaetanLepage · 2024-08-08T06:18:19Z

@Mergifyio queue

mergify · 2024-08-08T06:18:41Z

queue

🛑 The pull request has been removed from the queue `default`

The queue conditions cannot be satisfied due to failing checks.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

GaetanLepage · 2024-08-08T08:28:13Z

https://github.com/Mergifyio queue

mergify · 2024-08-08T08:28:18Z

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 78abafe

psfloyd force-pushed the lz.init branch from 648f988 to 03f90fa Compare July 16, 2024 05:54

psfloyd mentioned this pull request Jul 16, 2024

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

Draft

6 tasks

MattSturgeon reviewed Jul 16, 2024

View reviewed changes

plugins/pluginmanagers/lz-n.nix Outdated Show resolved Hide resolved

psfloyd force-pushed the lz.init branch from 8f7914b to 03f90fa Compare July 17, 2024 01:36

MattSturgeon requested changes Jul 17, 2024

View reviewed changes

psfloyd force-pushed the lz.init branch from f47c037 to f81ba4f Compare July 18, 2024 05:19

MattSturgeon reviewed Jul 18, 2024

View reviewed changes

plugins/pluginmanagers/lz-n.nix Outdated Show resolved Hide resolved

psfloyd force-pushed the lz.init branch from f81ba4f to 42d10e0 Compare July 21, 2024 02:33

MattSturgeon reviewed Jul 21, 2024

View reviewed changes

plugins/pluginmanagers/lz-n.nix Outdated Show resolved Hide resolved

MattSturgeon reviewed Jul 21, 2024

View reviewed changes

plugins/pluginmanagers/lz-n.nix Outdated Show resolved Hide resolved

MattSturgeon reviewed Jul 21, 2024

View reviewed changes

plugins/pluginmanagers/lz-n.nix Outdated Show resolved Hide resolved

MattSturgeon reviewed Jul 21, 2024

View reviewed changes

plugins/pluginmanagers/lz-n.nix Outdated Show resolved Hide resolved

MattSturgeon mentioned this pull request Jul 22, 2024

plugins/lazy.nvim: Import Statements & Adding user-specified packages to lazyPath #1904

Closed

psfloyd force-pushed the lz.init branch 2 times, most recently from e08c70d to a91767d Compare July 26, 2024 07:03

MattSturgeon requested changes Jul 26, 2024

View reviewed changes

MattSturgeon reviewed Jul 26, 2024

View reviewed changes

plugins/pluginmanagers/lz-n.nix Outdated Show resolved Hide resolved

psfloyd force-pushed the lz.init branch from a91767d to efd52d4 Compare July 27, 2024 01:51

psfloyd requested a review from MattSturgeon August 3, 2024 16:54

MattSturgeon reviewed Aug 3, 2024

View reviewed changes

psfloyd force-pushed the lz.init branch from efd52d4 to b816520 Compare August 3, 2024 23:28

MattSturgeon reviewed Aug 3, 2024

View reviewed changes

plugins/pluginmanagers/lz-n.nix Show resolved Hide resolved

GaetanLepage requested changes Aug 4, 2024

View reviewed changes

plugins/pluginmanagers/lz-n.nix Outdated Show resolved Hide resolved

psfloyd force-pushed the lz.init branch from b816520 to 673c848 Compare August 4, 2024 23:37

MattSturgeon approved these changes Aug 5, 2024

View reviewed changes

MattSturgeon requested review from traxys and GaetanLepage August 5, 2024 10:42

psfloyd force-pushed the lz.init branch from 673c848 to 9b67dc1 Compare August 8, 2024 01:57

GaetanLepage approved these changes Aug 8, 2024

View reviewed changes

plugins/lz-n: init

78abafe

GaetanLepage force-pushed the lz.init branch from 9b67dc1 to 78abafe Compare August 8, 2024 06:18

mergify bot merged commit 78abafe into nix-community:main Aug 8, 2024
4 checks passed

mergify bot temporarily deployed to github-pages August 8, 2024 08:28 Inactive

psfloyd deleted the lz.init branch August 8, 2024 23:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

plugins/lz-n: init #1874

plugins/lz-n: init #1874

psfloyd commented Jul 16, 2024

MattSturgeon left a comment

psfloyd commented Jul 17, 2024

MattSturgeon left a comment

psfloyd commented Jul 18, 2024

psfloyd commented Jul 21, 2024 •

edited

Loading

MattSturgeon left a comment

MattSturgeon Jul 26, 2024

MattSturgeon Jul 26, 2024

MattSturgeon left a comment

MattSturgeon Aug 3, 2024

psfloyd commented Aug 3, 2024

MattSturgeon commented Aug 3, 2024

MattSturgeon commented Aug 3, 2024

MattSturgeon commented Aug 4, 2024

psfloyd commented Aug 4, 2024

GaetanLepage left a comment

MattSturgeon left a comment

GaetanLepage left a comment

GaetanLepage commented Aug 8, 2024

mergify bot commented Aug 8, 2024 •

edited

Loading

GaetanLepage commented Aug 8, 2024

mergify bot commented Aug 8, 2024 •

edited

Loading

	listToUnkeyedAttrs =
	list:
	builtins.listToAttrs (lib.lists.imap0 (idx: lib.nameValuePair "__unkeyed-${toString idx}") list);

plugins/lz-n: init #1874

plugins/lz-n: init #1874

Conversation

psfloyd commented Jul 16, 2024

MattSturgeon left a comment

Choose a reason for hiding this comment

psfloyd commented Jul 17, 2024

MattSturgeon left a comment

Choose a reason for hiding this comment

psfloyd commented Jul 18, 2024

psfloyd commented Jul 21, 2024 • edited Loading

MattSturgeon left a comment

Choose a reason for hiding this comment

MattSturgeon Jul 26, 2024

Choose a reason for hiding this comment

MattSturgeon Jul 26, 2024

Choose a reason for hiding this comment

MattSturgeon left a comment

Choose a reason for hiding this comment

MattSturgeon Aug 3, 2024

Choose a reason for hiding this comment

psfloyd commented Aug 3, 2024

MattSturgeon commented Aug 3, 2024

MattSturgeon commented Aug 3, 2024

MattSturgeon commented Aug 4, 2024

psfloyd commented Aug 4, 2024

GaetanLepage left a comment

Choose a reason for hiding this comment

MattSturgeon left a comment

Choose a reason for hiding this comment

GaetanLepage left a comment

Choose a reason for hiding this comment

GaetanLepage commented Aug 8, 2024

mergify bot commented Aug 8, 2024 • edited Loading

🛑 The pull request has been removed from the queue default

GaetanLepage commented Aug 8, 2024

mergify bot commented Aug 8, 2024 • edited Loading

✅ The pull request has been merged automatically

psfloyd commented Jul 21, 2024 •

edited

Loading

mergify bot commented Aug 8, 2024 •

edited

Loading

🛑 The pull request has been removed from the queue `default`

mergify bot commented Aug 8, 2024 •

edited

Loading