Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib/helpers: refactor mkPlugin helpers #1717

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

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 !

lib/plugin/neovim-plugin.nix Outdated Show resolved Hide resolved
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;

lib/plugin/neovim-plugin.nix Outdated Show resolved Hide resolved
lib/plugin/vim-plugin.nix Outdated Show resolved Hide resolved
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