plugins/lazy.nvim: switch to mkNeovimPlugin

[my7h3le]

This PR came about as a result of the comments made in this PR #1904. It refactors the existing lazy plugin manager to use mkNeovimPlugin and makes the plugin type freeform.

Closes #2174 #1904

Here's an example of how to set up LazyVim using the changes to this plugin (Note: this is a standalone flake, saving it to any directory as flake.nix and running nix build should be enough, to launch neovim run ./result/bin/nvim)

# flake.nix

{
  description = "Setup LazyVim using NixVim";

  inputs.nixpkgs.url = "github:NixOS/nixpkgs/nixos-unstable";
  inputs.nixvim.url = "github:my7h3le/nixvim/lazy-plugin-manager-freeform";
  inputs.nixvim.inputs.flake-parts.follows = "flake-parts";
  inputs.flake-parts.url = "github:hercules-ci/flake-parts";
  inputs.flake-parts.inputs.nixpkgs-lib.follows = "nixpkgs";

  outputs =
    {
      self,
      nixpkgs,
      nixvim,
      flake-parts,
    }@inputs:
    flake-parts.lib.mkFlake { inherit inputs; } {
      systems = [
        "aarch64-darwin"
        "aarch64-linux"
        "x86_64-darwin"
        "x86_64-linux"
      ];

      perSystem =
        {
          pkgs,
          lib,
          system,
          ...
        }:
        let
          config = {

            # This path is required to be added at runtime at the beginning of
            # the `init.lua` file for `nvim-treesitter`'s parser_install_dir
            # option to work correctly. Unfortunately `nvim-treesitter`
            # requires the parser installation directory to be read-writeable
            # so it can't be in a path in the nix store.
            extraConfigLuaPre = ''
              vim.opt.runtimepath:prepend(vim.fs.joinpath(vim.fn.stdpath("data"), "site"))
            '';

            plugins = {
              lazy = {
                enable = true;
                settings = {
                  defaults = {
                    # When true `lazy.nvim` will lazy load all plugins by default (`lazy.nvim` sets this
                    # to false by default). 
                    lazy = false;
                  };
                  install = {
                    # Don't install missing plugin dependencies.
                    missing = false;
                  };
                  dev = {
                    # Prevent `lazy.nvim` from fallbacking to fetch plugins
                    # from GitHub. You can set this to true as well.
                    fallback = false;
                  };
                  performance = {
                    rtp = {
                      # Disable some rtp plugins
                      disabled_plugins = [
                        "gzip"
                        "matchit"
                        "matchparen"
                        "netrwPlugin"
                        "tarPlugin"
                        "tohtml"
                        "tutor"
                        "zipPlugin"
                        "gzip"
                        "tarPlugin"
                        "tohtml"
                        "zipPlugin"
                      ];
                    };
                  };
                };

                plugins =
                  with pkgs.vimPlugins;
                  [
                    {
                      pkg = LazyVim;
                      import = "lazyvim.plugins";
                      # LazyVim needs `trouble-nvim` specified as a dependency
                      dependencies = trouble-nvim;
                    }
                    {
                      pkg = nvim-treesitter;
                      opts = {
                        ensure_installed = [
                          "vim"
                          "regex"
                          "lua"
                          "bash"
                          "markdown"
                          "markdown_inline"
                        ];
                        highlight = {
                          enable = true;
                        };
                        parser_install_dir.__raw = ''vim.fs.joinpath(vim.fn.stdpath("data"), "site")'';
                      };
                    }
                    {
                      pkg = telescope-fzf-native-nvim;
                      enabled = true;
                    }
                    {
                      pkg = mason-nvim;
                      enabled = false;
                    }
                    {
                      pkg = mason-lspconfig-nvim;
                      enabled = false;
                    }
                    luvit-meta
                    bufferline-nvim
                    grug-far-nvim
                    tokyonight-nvim
                    cmp-buffer
                    cmp-nvim-lsp
                    cmp-path
                    trouble-nvim
                    conform-nvim
                    dashboard-nvim
                    dressing-nvim
                    flash-nvim
                    friendly-snippets
                    gitsigns-nvim
                    indent-blankline-nvim
                    lazydev-nvim
                    lualine-nvim
                    neo-tree-nvim
                    noice-nvim
                    nui-nvim
                    nvim-autopairs
                    nvim-cmp
                    nvim-lint
                    nvim-lspconfig
                    nvim-notify
                    nvim-ts-autotag
                    nvim-snippets
                    nvim-spectre
                    nvim-treesitter-textobjects
                    persistence-nvim
                    plenary-nvim
                    telescope-nvim
                    todo-comments-nvim
                    ts-comments-nvim
                    which-key-nvim
                  ]
                  ++
                    # nix bundles all `mini-nvim` modules together, but LazyVim
                    # expects individual `mini.nvim` modules so map their
                    # individual names to the bundled `mini-nvim` package.
                    lib.map
                      (mini-module: {
                        name = "${mini-module}";
                        pkg = pkgs.vimPlugins.mini-nvim;
                      })
                      [
                        "mini.ai"
                        "mini.icons"
                        "mini.move"
                        "mini.nvim"
                        "mini.pairs"
                        "mini.splitjoin"
                        "mini.trailspace"
                      ];
              };
            };

          };
          nixvim' = nixvim.legacyPackages."${system}";
          nvim = nixvim'.makeNixvim config;
        in
        {
          packages = {
            inherit nvim;
            default = nvim;
          };
        };
    };
}

[MattSturgeon]

Seems there's two errors being picked up by CI currently, both part of the test-17 group:

error: builder for '/nix/store/XXX-empty.drv' failed with exit code 1;
       last 8 log lines:
       > ERROR: Error detected while processing /nix/store/6kqddn2g51j1lvbqsv6v17f6565ihpbk-init.lua:
       > Invalid plugin spec {
       >   dev = {
       >     fallback = false,
       >     path = "/nix/store/k71lsglx88q8ycaqj8vrj56hh80r4ccw-lazy-plugins",
       >     patterns = { "." }
       >   }
       > }

And

error: builder for '/nix/store/XXX-test.drv' failed with exit code 1;
       last 1 log lines:
       > ERROR: gitsigns: git not in path. Aborting setup

[my7h3le]

test-17 group

Hmmm I did see that and I think I have some ideas on how to fix it. Do you know how to run just the test-17 group? This doesn't work nix flake check checks.aarch64-linux.test-17 and I've been running nix flake check --all-systems

[MattSturgeon]

Do you know how to run just the test-17 group?

You can do nix build .#checks.<system>.test-17 (replace <system> as appropriate).

If you enter a devshell, you can also use our tests command to run a specific test from within that group, e.g. interactively with tests -i or by specifying the test name on the command line tests plugins-pluginmanagers-lazy.

Further, if you don't want to use a devshell, you can still get at individual tests within a group by using the entries passthru (tab-completion can help here).

E.g. nix build .#checks.x86_64-linux.test-17.entries.plugins-pluginmanagers-lazy --show-trace

[MattSturgeon]

So I was able to simplify the pluginToLua function, I'll add that as a temporary commit to this PR, and if it's deemed as appropriate for this PR itself I can squash it but if not I'll move it into it's own separate PR

Looks much better. I think it makes sense as part of this PR.

I only really went over this part of the PR in this review, but I'll try and find time to fully review soon as everything seems to be coming together 😀

[khaneliman]

I'm sure @MattSturgeon will have better recommendations for wording of descriptions

[khaneliman]

Suggested change

	When false then this plugin will not be included in the spec. (accepts fun():boolean)
	When false then this plugin will not be included in the spec.

The type will explain this

[MattSturgeon]

The type won't fully explain this; it won't show the expected function signature (i.e. no parameters -> returns a bool)

[khaneliman]

These could all do with examples, probably..

[khaneliman]

Suggested change

	mkNullOrOption (maybeRaw (attrsOf anything)) ''
	opts should be a table (will be merged with parent specs),
	return a table (replaces parent specs) or should change a table.
	The table will be passed to the Plugin.config() function.
	Setting this value will imply Plugin.config()
	'';
	mkNullOrOption (maybeRaw (attrsOf anything)) ''
	The opts value can be one of the following:
	- A table: This table will be merged with any existing configuration settings from parent specifications.
	- A function that returns a table: The returned table will completely replace any existing configuration settings from parent specifications.
	- A function that modifies a table: This function will receive the existing configuration table as an argument and can modify it directly.
	In all cases, the resulting configuration table will be passed to the Plugin.config() function.
	Setting the opts value automatically implies that Plugin.config() will be called.
	'';

I think this might make it more clear what is occurring? Maybe we should link to their docs, too?

[my7h3le]

Yeah that's a lot easier to understand compared to the offical docs. I will add that on an unrelated note how are you wrapping your comments, just gw (gq doesn't work for me) in a blank file? When I do gw for the comment I think it's taking the indentation into account so each line becomes shorter so instead of:

Optional specs are only included in the final configuration if the
corresponding plugin is also specified as a required (non-optional) plugin
elsewhere. This feature is particularly helpful for Neovim distributions,
allowing them to pre-configure settings for plugins that users may or may not
have installed.

I get:

                  Optional specs are only included in the final configuration
                  if the corresponding plugin is also specified as a required
                  (non-optional) plugin elsewhere. This feature is
                  particularly helpful for Neovim distributions, allowing
                  them to pre-configure settings for plugins that users may
                  or may not have installed.

Or is what i'm getting what it's supposed to be?

[MattSturgeon]

That sounds like lazy.nvim is telling us this option can be used to configure plugins that may or may not be loaded.

It is saying that a different part of lazy's configuration is responsible for saying what/when plugins are loaded.

[MattSturgeon]

Re: gw and gq, I generally wrap manually. I usually try to wrap semantically; preferring line breaks at punctuation or the end of a phrase if possible. Unless it looks too silly and unbalanced.

[my7h3le]

That sounds like lazy.nvim is telling us this option can be used to configure plugins that may or may not be loaded.

It is saying that a different part of lazy's configuration is responsible for saying what/when plugins are loaded.

Yup essentially. So if the plugin isn't installed lazy.nvim won't fetch it, and if it is it won't load unless another spec tags it as not optional and defines the conditions for it to be loaded (or the other spec has enabled = true)

[khaneliman]

So I was able to simplify the pluginToLua function, I'll add that as a temporary commit to this PR, and if it's deemed as appropriate for this PR itself I can squash it but if not I'll move it into it's own separate PR

Looks much better. I think it makes sense as part of this PR.
I only really went over this part of the PR in this review, but I'll try and find time to fully review soon as everything seems to be coming together 😀

Thanks for taking the time to look over it!

Thanks for taking this module on, it looks like a beast of one!

[MattSturgeon]

How's this going functionally? Are you still running into issues getting the correct store paths assigned to plugin dir? Do you need help with anything?

[my7h3le]

How's this going functionally? Are you still running into issues getting the correct store paths assigned to plugin dir? Do you need help with anything?

Functionally everything works and I'm not running into those issues anymore (#2082 (comment) the fix was this #2082 (comment)) , and we don't need to update/overlay nixpkgs to get this PR to work (I still think it's worthwhile doing regardless). I was going to add an example of how to set up LazyVim in settingsExample but I'll also create a standalone flake for it and link it here so that you guys can run it.

I guess right now the only things I'd need help with is the review, and any rewording of the docs :) I'll reference any lines or blocks that I'd want input on as well.

I’ll also explicitly be adding some settings options that I think will help for visibility

[my7h3le]

So I tried to have the pkg attribute be stripped around here

nixvim/plugins/pluginmanagers/lazy.nix

Line 162 in bb8135f

config.name = lib.mkIf (cfg.pkg != null) (lib.mkDefault "${lib.getName cfg.pkg}");

but I couldn't get it to work. I tried doing config.pkg = lib.mkIf (config.pkg != null) lib.mkDefault null but that leads to infinite recursion. Is there a better way to strip the pkg attribute after it's done being used or is this approach fine?

[MattSturgeon]

Have you double checked nixvim-print-init? By default toLuaObject filters out any null or empty items.

Yes, this is a horrible hack, but it's currently the only reasonable way we have to filter out null sub-options from freeform submodules...

[my7h3le]

So pkg can be not null because a user can specify a package using the pkg attribute. lazy.nvim doesn't have a pkg attribute for plugin specs, it's just something we have for convenience to map nixpkgs to the lazy.nvims dir property. So all pkg attributes should be set to null after they've been mapped to the dir attribute. Currently I'm doing that at the end via removePkgAttrFromPlugins at the end in extraConfig but was just wondering if there's a better way to do this

[MattSturgeon]

I'll have a look at the actual code a bit later, but it sounds like we need a builtins.removeAttrs plugin [ "pkg" ] somewhere when we convert cfg.plugins -> settings.spec.

[my7h3le]

Yup pretty much. I tried doing the conversion in cfg.plugins after the dir attribute was set but that didn't work and lead to infinite recursion

[MattSturgeon]

My view is (unlike settings) plugins isn't aiming to be a 1-to-1 mapping of the upstream config format.

Instead it is an alternative (nicer) way to define settings.spec, whatever tranformations need to be done to it should be done when defining plugins.lazy.settings.spec.

This means users will have a predictable value at config.plugins.lazy.plugins if they want to read that in their own modules for any reason.

Dumb exercise in what's possible

To be clear, I'm not recommend you use apply here.

But for the sake argument, if you did want to transform the value at config.plugins.plugins to be different in some way from what users define, this could be done with either a custom option-type's merge function (this is how coercedTo works) or defining an apply function when calling mkOption.

apply acts as a final transformation on the fully merged option value, so if I wanted to I could do something really stupid like:

{ config, lib, ... }:
{
  # Declare foo as a boolean option, that converts its final value to a stupid string...
  options.foo = lib.mkOption {
    type = types.bool;
    apply = v: "was defined as ${lib.boolToString v}!";
  };

  config = {
    # Define foo to be true
    foo = true;

    assertions = [
      {
        # NOTE: while `foo` is defined as a bool, its value is a string!
        # DO NOT DO THIS lol
        assertion = config.foo == "was defined as true!";
        message = "apply failed";
      }
    ];
  };
}

Note that this could be used as a sledgehammer to work around infinite recursion, but is probably an anti-pattern 9 times out of 10.

[my7h3le]

I updated the first comment with an example standalone flake that sets up LazyVim that you guys can try. I'll add it as an example to the plugin as well. Let me know if it should be moved elsewhere instead of the first comment

[my7h3le]

I had issues lazy-loading on keys when I was writing an example for the docs on how to use this trigger. The type for this can be made better to, I'll try to come up with something better as it's a common use case to lazy load on keybindings

[MattSturgeon]

Suggested change

	config.name = lib.mkIf (cfg.pkg != null) (lib.mkDefault "${lib.getName cfg.pkg}");
	config.dir = lib.mkIf (cfg.pkg != null) (lib.mkDefault "${cfg.pkg}");
	config = lib.mkIf (cfg.pkg != null) {
	name = lib.mkDefault "${lib.getName cfg.pkg}";
	dir = lib.mkDefault "${cfg.pkg}";
	};

[MattSturgeon]

TBH I don't think we even need separate pkg and dir options...

Couldn't we have a source option that is either path package?

We could still have a default for name, e.g:

config.name = mkOptionDefault (
  if isDerivation config.source then
    getName config.source
  else
    baseNameOf config.source
);

I'm also debating mkDefault vs mkOptionDefault here. The latter is lower priority (higher priority number); its the same as a default passed directly to mkOption.

[my7h3le]

We could have a source option that is either path package but it would have to be resolved to dir in the end for lazy.nvim to pick it up. Since the pluginType is also freeform, the user can still specify dir if they want to so we'd have to deal with potential collisions when the user specifies both source and dir, but I mean for that case we could just do config.dir = (cfg.dir == nulll && cfg.source !=null) lib.mkDefault "${cfg.source}". But at that point wouldn't source just be another abstraction for pkg?

[my7h3le]

So the thing I've found out is if source is restricted to just be of type path url short git url or package, the case that gets missed is if a user is trying to disable a plugin via a custom name.

Being able to disable a plugin by a custom name is important. One such instance of why this is important is because there are several vim plugins that get bundled together in one nix package (mini-nvim for example that bundles in all mini modules such as mini-ai, mini-trailspace etc), and you won't be able to disable an individual plugin without affecting other bundled plugins if you can't do something like:

{
  source = "mini.ai";
  enabled = false;
}

One workaround would be to use the name option but I feel like the intention of the name option would then differ from upstream. Allowing source to be just a string would mean less type validation though. I guess we can take a garbage in garbage out approach and let all the validation be handled by upstream lazy.nvim but I'm not really sure what would be ideal here. Any thoughts?

[MattSturgeon]

That sounds like a valid reason to have it accept any string. Makes me wonder if "source" is an appropriate name though, if it can also be a "plugin name"?

If we like the idea of a type-checked option named source, we could also have a "name" option. Bearing in mind it doesn't necessarily have to correspond 1-to-1 with upstream's plugin spec, since we can do conversion when assigning to settings.spec.

No strong preferences here, just exploring possible options 🙂

[MattSturgeon]

This is concerning... In particular:

Also use types.eitherRecursive instead of types.either here, as using just types.either also leads to stack overflow.

eitherRecursive and either are usually the same thing; the former only exists for documentation purposes (and should probably be deprecated in favour of checking config.isDocs).

nixvim/lib/types.nix

Lines 98 to 99 in fc7e9b2

	# Overridden when building the documentation
	eitherRecursive = types.either;

If eitherRecursive has a different behaviour to either outside of our docs, then something is very broken.

[MattSturgeon]

I would personally prefer to implement this something like:

dependenciesType =
  if config.isDocs then
    # Use a stub type for documentation purposes
    # We don't need to repeat all the plugin-type sub-options again in the docs
    # It'd also be infinitely recursive
    lib.mkOptionType {
      description = "plugin submodule";
      descriptionClass = "noun";
      check = throw "should not be used";
      merge = throw "should not be used";
    }
  else
    lazyPluginType;
in
  dependencies = mkOption {
    # NOTE: use attrsOf for the LHS, because coercedTo only supports submodules on the RHS
    type = types.coercedTo (attrsOf anything) lib.toList (listOf dependenciesType);
  };

Note: I'm fine with non-nullable lists/attrs here and using coercedTo, because this is our option, not a direct representation of upstream's settings.

[my7h3le]

Hmmm so i'm getting errors saying that isDocs is missing. Doing ((config.isDocs or null) != null) while it works (at least for tests) shouldn't need to be done right?

[devshell]$ tests plugins-pluginmanagers-lazy
Running 1 tests: plugins-pluginmanagers-lazy
error:
       … while calling the 'derivationStrict' builtin

         at /builtin/derivation.nix:9:12: (source not available)

       … while evaluating derivation 'plugins-pluginmanagers-lazy'
         whose name attribute is located at /nix/store/887hpp8a2i99n9jjwcvz6qkhhhqsvzkg-source/pkgs/stdenv/generic/make-derivation.nix:336:7

       … while evaluating attribute 'buildCommand' of derivation 'plugins-pluginmanagers-lazy'

         at /nix/store/887hpp8a2i99n9jjwcvz6qkhhhqsvzkg-source/pkgs/build-support/trivial-builders/default.nix:59:16:

           58|         enableParallelBuilding = true;
           59|         inherit buildCommand name;
             |                ^
           60|         passAsFile = [ "buildCommand" ]

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

       error: attribute 'isDocs' missing

       at /nix/store/94m1msjw2jq997gqb38zgb2l9669xp3q-source/plugins/pluginmanagers/lazy.nix:142:26:

          141|                     dependenciesType =
          142|                       if config.isDocs then
             |                          ^
          143|                         # Use a stub type for documentation purposes
Test failure

This is currently what I temporarily have inside the submodule:

                dependencies =
                  let
                    dependenciesType =
                      if config.isDocs then
                        # Use a stub type for documentation purposes
                        # We don't need to repeat all the plugin-type sub-options again in the docs
                        # It'd also be infinitely recursive
                        lib.mkOptionType {
                          description = "plugin submodule";
                          descriptionClass = "noun";
                          check = throw "should not be used";
                          merge = throw "should not be used";
                        }
                      else
                        lazyPluginType;
                  in
                  # NOTE: use attrsOf for the LHS, because coercedTo only supports submodules on the RHS
                  mkNullOrOption (types.coercedTo (attrsOf anything) lib.toList (listOf dependenciesType)) ''
                    A list of plugin names or plugin specs that should be
                    loaded when the plugin loads. Dependencies are always
                    lazy-loaded unless specified otherwise. When specifying a
                    name, make sure the plugin spec has been defined somewhere
                    else. This can also be a single string such as for a short
                      plugin url (See: https://lazy.folke.io/spec).
                  '';

[MattSturgeon]

Looks like you're using the submodule's config which won't have isDocs.

The isDocs option is declared in the programs.nixvim submodule, so it won't exist in your submodule.

[my7h3le]

Ooohh thanks! yup that fixed things! I'll test if this causes infinite recursion on my home-manager setup and I'll get the changes we talked about minus some updates to the docstrings (will get around to updating them once the implementation itself is finalized) soon ish. Feel free to still make suggestions on the docstrings though (I will get around to them 🙂)

[MattSturgeon]

Just a reminder: settings is a freeform option. This means we don't need to declare sub-options for everything that can be configured.

Any config definitions that don't match the options we've declared will use the freeformType attrsOf anything.

[my7h3le]

Yeah so these were only added purely for documentation purposes, as some of the options in the plugin type reference these settings. I can totally remove them though if it's deemed better to avoid having them 🙂

[MattSturgeon]

I can totally remove them though if it's deemed better to avoid having them 🙂

It's always a difficult judgement call. Let's revisit this towards the end of the review process, unless anyone already has a strong opinion.

[my7h3le]

I'll stash and remove them for now then, @khaneliman do you have any thoughts on this?

[MattSturgeon]

After going over everything again, I don't think there were as many settings options as I thought. With this dropped, there's currently zero.

I'm leaning towards including at least some of them 🫡

[my7h3le]

These are all the settings options lazy.nvim has https://lazy.folke.io/configuration but only dev.fallback, install.missing and git.url_format is worth including. git.url_format may not even be worth including as it's more purely just for documentation purposes. dev.fallback and install.missing would be worth it as we'd be using different defaults compared to upstream, because by default we shouldn't fetch packages out out of tree but prefer to use nix packages.

[khaneliman]

I always prefer having a decent amount of settings included to make it obvious what functionality can be customized without going to upstream docs. But, some plugins have too many that it becomes just pure tech debt to support all the settingsOptions.

[MattSturgeon]

Apologies if I seem a bit confused and/or inconsistent in this review. This is quite a complex plugin (and refactor) to wrap my head around!

[MattSturgeon]

Suggested change

	let
	cfg = config;
	in

For the sake of 3 chars, let's not alias this argument. Sometimes abstractions and indirection just add unnecessary complexity 😉

[MattSturgeon]

Why str? Don't we want path here? (docs)

Also, I think path should match derivations, so we wouldn't need either path package, simply path should be ok.

[my7h3le]

So it can be just a name as well because [1] usptream can be a name or url. If it is a name the spec has to be defined somewhere else in the list. The current implementation each element in the plugins list has the same type as [1] upstream, so each element can be a (name | url | short git url | dir). We either make each element in the list be either package | path | url or explicitly package | path and if a user wants a url they have to specify it in an attribute set but personally I would prefer the former (package | path | url). Removing it being a name does make sense though as having a name isn't entirely useful as other properties for the plugin would have to be defined later in the list with an attribute set

[MattSturgeon]

Removing it being a name does make sense though as having a name isn't entirely useful as other properties for the plugin would have to be defined later in the list with an attribute set

Good observation. I agree it makes no sense to coerce a name string to an attrset if a name definition without any other config is effectively useless. If a user really wanted to, they could still do it manually with settings.spec or plugins.*.name (if we have/keep that).

[my7h3le]

Removing it being a name does make sense though as having a name isn't entirely useful as other properties for the plugin would have to be defined later in the list with an attribute set

Good observation. I agree it makes no sense to coerce a name string to an attrset if a name definition without any other config is effectively useless. If a user really wanted to, they could still do it manually with settings.spec or plugins.*.name (if we have/keep that).

I think keeping plugins.*.name makes sense in case the user wants to use a very custom name that is arbitrary, but the plugins.*.source option will just set a default value for name.

Edit: actually it might not make sense to have plugins.*.name reading the spec again https://lazy.folke.io/spec plugins.*.name is just a display name and or the directory name (when lazy.nvim fetches it from git) it doesn't actually change what name you pass to require(<plugin-name>). So I think it does make sense to remove plugins.*.name as well

[MattSturgeon]

Suggested change

freeformType = attrsOf anything;

Freeform types are great for dealing with arbitrary config, but I'm skeptical we should have one in a custom option where we're re-defining the interface to be more nix-friendly?

Not a strong opinion atm, more a gut feeling.

[my7h3le]

So there are a fair amount of plugin spec options we don't explicitly implement like import specs version branch etc. https://lazy.folke.io/spec Removing the freeformType would mean we'd have to explicitly implement at least some of those options (the import plugin spec option is important). Also implementing specs (each plugin can have a specs option) ourselves might be annoying

[MattSturgeon]

What feels weird to me is that this is simultaneously a freeform type, where users expect their definitions to be used verbatim, and also a wrapper abstraction on top of settings.spec.

The two seem contradictory to me.

We could go "old-school" and have an extraOptions option, but that feels like a bad solution...

It'll probably be fine if we just document the transformation clearly, but it still feels off.

[my7h3le]

So can the plugin type itself have extraOptions so would it look something like this?

plugins.lazy.plugins = with pkgs.vimPlugins; [
  { 
    source = vim-closer;
    # Extra options used when user wants to pass additional definitions verbatim to the plugin spec?
    extraOptions = { import "vim-closer"; };
  }
]

[MattSturgeon]

Yes, if we declare options.extraOptions = mkOption { type = attrsOf anything; } instead of a freeformType.

As I said, it's a bit old school...

[MattSturgeon]

Oh, I see. You were using str to mean the plugin name? I'm not sure I'm a fan of this. Because derivations are string-like types this could get confusing fast.

I think it best to either do no coercion at all, and always expect a submodule definition, or to only coerce one type (e.g. types.path).

[my7h3le]

So at the time I didn't know I could have a url type, so I just used str as a catch all for either (name | dir | url | short git plugin-url ) 😅

[MattSturgeon]

If this is a custom submodule in a custom option, there's no need to be making users define unkeyed options.

Let's give this a useful name and then convert it later when assigning this option's value to settings.spec.

[MattSturgeon]

IMO if the plugins option simply matches the schema of settings.spec, without offering any abstractions, then it is redundant and should be replaced with a rename alias pointing to settings.spec.

[MattSturgeon]

I'm confused. Both dir and __unkeyed say they can be the path to a directory where the plugin is found... Are they the same thing?

[MattSturgeon]

Ah I see, you had url pkg and dir all mapping internally to __unkeyed if defined?

In this case I think we just remove the __unkeyed option from this submodule and do that kinda conversion later, when defining settings.spec.

[my7h3le]

Yeah it's weird upstream [1] or __unkeyed in our case is a catch all almost as it can be a name, url, short git url, or a directory https://lazy.folke.io/spec. I don't internally map url, dir, pkg to __unkeyed, but rather lazy.nvim handles all the different cases for __unkeyed or [1], and if a dir or url is given those get passed directly to lazy.nvim as well and lazy.nvim decides what too pick (hopefully I explained that somewhat well)

[my7h3le]

But speaking of if we were to remove __unkeyed and internally map dir, name, url to __unkeyed at the end when the lua conversion is about to occur that could be cleaner. That would also mean we can have a source option and omit the dir, pkg and url options (it would differ from upstream but I'm assuming that is fine). source at the end before the conversion would get mapped to __unkeyed

[MattSturgeon]

EDIT for clarity: this was written before seeing the above comment.

Gotcha. So these options all map to upstream options, but there's some redundancy?

I think we need to better understand how upstream handles these before finalizing a design here.

As for __unkeyed I feel like it can be named something better if this is an option we're intentionally doing transformations to, since our usual rules of "pass exactly what the user configured in settings to setup" don't really apply here. 🤔

[my7h3le]

Yeah they're all upstream options, lazy.nvim. But instead of redundancy I think they did it more for convenience. It's convenient in lua to just pass a (url | short git url | dir | name) as just one argument to [1], and I think they added explicit dir url options for mild performance gains as in if a user uses dir for example, then don't bother parsing and determining if it's a directory/url/name

[my7h3le]

I think getting rid of unkeyed and making it so each element in the plugins list is either path | url | package or lazyPluginType where lazyPluginType is the submodule type

[MattSturgeon]

I think we can combine the various "source" options into a single one. Or at the very least fewer.

First let's double-check if types.path is in fact a superset of types.package, if so that simplifies things. Especially if the un-stringified derivation attrs survive the type merging process.

We can also use a "url" type if needed, types.strMatching "https?://.*" for example.

I'm imaging something like:

source = mkOption {
  type = with types; oneOf [
    path
    package
    (strMatching "https?://.*")
  ];
}

[MattSturgeon]

I'm open to discussion on this. Is there benefit to having multiple? Or do they map to several options in the upstream plugin spec?

[my7h3le]

So types.package seems to be distinct and not a subest of types.path https://github.com/hsjobeki/nixpkgs/blob/4c181b62b7615af4cd0d139d5b82019fe10015d4/lib/types.nix#L532
https://github.com/hsjobeki/nixpkgs/blob/4c181b62b7615af4cd0d139d5b82019fe10015d4/lib/types.nix#L501

    path = mkOptionType {
      name = "path";
      descriptionClass = "noun";
      check = x: isStringLike x && builtins.substring 0 1 (toString x) == "/";
      merge = mergeEqualOption;
    };

    pathInStore = mkOptionType {
      name = "pathInStore";
      description = "path in the Nix store";
      descriptionClass = "noun";
      check = x: isStringLike x && builtins.match "${builtins.storeDir}/[^.].*" (toString x) != null;
      merge = mergeEqualOption;
    };
    
    # A package is a top-level store path (/nix/store/hash-name). This includes:
    # - derivations
    # - more generally, attribute sets with an `outPath` or `__toString` attribute
    #   pointing to a store path, e.g. flake inputs
    # - strings with context, e.g. "${pkgs.foo}" or (toString pkgs.foo)
    # - hardcoded store path literals (/nix/store/hash-foo) or strings without context
    #   ("/nix/store/hash-foo"). These get a context added to them using builtins.storePath.
    # If you don't need a *top-level* store path, consider using pathInStore instead.
    package = mkOptionType {
      name = "package";
      descriptionClass = "noun";
      check = x: isDerivation x || isStorePath x;
      merge = loc: defs:
        let res = mergeOneOption loc defs;
        in if builtins.isPath res || (builtins.isString res && ! builtins.hasContext res)
          then toDerivation res
          else res;
    };

There's also a pathInStore option type as well.

[my7h3le]

I actually like have just a single source option and then just mapping it to __unkeyed at the end when the conversion is about to occur. Despite it being different than upstream it would be cleaner, and simplify some test cases too

[MattSturgeon]

package is a top-level store path (/nix/store/hash-name)

i.e. its a subset of pathInStore:

path in the Nix store

which itself is a subset of path:

isStringLike && firstChar == "/"

[my7h3le]

I'm open to discussion on this. Is there benefit to having multiple? Or do they map to several options in the upstream plugin spec?

I don't think it makes sense for us to have multiple attributes to define a plugin source. I mean we can but I think having a source option is cleaner if we can check if the given option value is either a url | package | path and then conditionally map them to dir url upstream options at the end before the conversion.

So being able to do this with final implementation I think would be nice:

plugins.lazy.plugins = with pkgs.vimPlugins; [
  vim-closer
  "https://github.com/echasnovski/mini.ai"
  "echasnovski/mini.trailspace"
  {
    source = dial-nvim;
       .
       .
    }
];

[my7h3le]

package is a top-level store path (/nix/store/hash-name)

i.e. its a subset of pathInStore:

path in the Nix store

which itself is a subset of path:

isStringLike && firstChar == "/"

Yup my bad I thought you meant if the types themselves literally extended types.path but just added some additional attributes/functionality

[MattSturgeon]

Suggested change

	lazyPluginsListType = types.listOf lazyPluginType;

This can just be defined inline as e.g. type = listOf lazyPluginType

[MattSturgeon]

This option would benefit greatly from an example and also a fleshed out description.

[my7h3le]

Yup I totally agree, I will get around to adding that, probably towards the final stages of the PR as it seems like plugins itself is going to change in definition

[MattSturgeon]

Suggested change

	plugins.lazy.settings.spec = removePkgAttrFromPlugins cfg.plugins;
	plugins.lazy.settings.spec = map toPluginSpec cfg.plugins;

Let's inline removePkgAttrFromPlugins here and rename removePkgAttrFromPlugin to toPluginSpec.

[my7h3le]

Apologies if I seem a bit confused and/or inconsistent in this review. This is quite a complex plugin (and refactor) to wrap my head around!

No worries at all! I was definitely pretty confused as well (still am to a certain extent 😂), and your comments helped a bunch! Besides I'm not in any rush to get this out, and I'd rather have it thoroughly review 🙂