Skip to content

Conversation

@GaetanLepage
Copy link
Member

No description provided.

Copy link
Member

@MattSturgeon MattSturgeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for the number of comments, you've probably already considered most of them.

Let's discuss tomorrow!

Comment on lines 9 to 28
mkPlugin = import ./mk-plugin.nix { inherit lib nixvimOptions nixvimUtils; };

neovim-plugin = import ./neovim-plugin.nix {
inherit
lib
nixvimOptions
nixvimUtils
toLuaObject
mkPlugin
;
};

vim-plugin = import ./vim-plugin.nix {
inherit
lib
nixvimOptions
nixvimUtils
mkPlugin
;
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be tempted to restructure the scopes a little:

  plugins = {
    mkPlugin = import ./mk-plugin.nix { inherit lib nixvimOptions nixvimUtils; };
    inherit (neovim-plugin) mkNeovimPlugin;
    inherit (vim-plugin) mkVimPlugin;
  };

  # Deprecated scope, use plugins.mkNeovimPlugin going forward
  neovim-plugin = import ./neovim-plugin.nix {
    inherit
      lib
      nixvimOptions
      nixvimUtils
      toLuaObject
      mkPlugin
      ;
  };

  # Deprecated scope, use plugins.mkVimPlugin going forward
  vim-plugin = import ./vim-plugin.nix {
    inherit
      lib
      nixvimOptions
      nixvimUtils
      mkPlugin
      ;
  };

Comment on lines 23 to 24
# TODO: DEPRECATED: use the `settings` option instead
extraOptionsOptions = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is slowly improving:

$ rg 'extraOptionsOptions' --files-with-matches | wc -l
68

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. Once the rest of the internal refactors will be over (soon as hope), we can focus on modernizing the rest of the codebase.

mkRenamedOptionModule (basePluginPath ++ [ "extraConfig" ]) settingsPath
));

extraOptions = settingsOption // (args.extraOptions or { });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you declare default args, you can just do // extraOptions, unless this attrs is recursive.

Suggested change
extraOptions = settingsOption // (args.extraOptions or { });
extraOptions = settingsOption // extraOptions;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed !

inherit extraPackages;
}
(extraConfig cfg)
(optionalAttrs (isColorscheme && (colorscheme != null)) { colorscheme = mkDefault colorscheme; })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this handle opt.termguicolors too?

Comment on lines 59 to 63
inherit description url;
path = [
namespace
name
];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
inherit description url;
path = [
namespace
name
];
inherit description url path;

Let's define this in a wider scope or as a default-arg so we can use it here and in the deprecations.


imports = optionsRenamedToSettingsWarnings ++ imports;

options.${namespace}.${name} =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
options.${namespace}.${name} =
options = setAttrByPath path (

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You find this clearer ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's more flexible, if we ever support having a nested namespace/path


config =
let
cfg = config.${namespace}.${name};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
cfg = config.${namespace}.${name};
cfg = getAttrFromPath path config;

@khaneliman khaneliman marked this pull request as draft January 31, 2025 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants