-
-
Notifications
You must be signed in to change notification settings - Fork 297
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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!
lib/plugin/default.nix
Outdated
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 | ||
; | ||
}; |
There was a problem hiding this comment.
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
;
};
lib/plugin/neovim-plugin.nix
Outdated
# TODO: DEPRECATED: use the `settings` option instead | ||
extraOptionsOptions = { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
lib/plugin/vim-plugin.nix
Outdated
mkRenamedOptionModule (basePluginPath ++ [ "extraConfig" ]) settingsPath | ||
)); | ||
|
||
extraOptions = settingsOption // (args.extraOptions or { }); |
There was a problem hiding this comment.
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.
extraOptions = settingsOption // (args.extraOptions or { }); | |
extraOptions = settingsOption // extraOptions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed !
lib/plugin/mk-plugin.nix
Outdated
inherit extraPackages; | ||
} | ||
(extraConfig cfg) | ||
(optionalAttrs (isColorscheme && (colorscheme != null)) { colorscheme = mkDefault colorscheme; }) |
There was a problem hiding this comment.
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?
lib/plugin/mk-plugin.nix
Outdated
inherit description url; | ||
path = [ | ||
namespace | ||
name | ||
]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
lib/plugin/mk-plugin.nix
Outdated
|
||
imports = optionsRenamedToSettingsWarnings ++ imports; | ||
|
||
options.${namespace}.${name} = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
options.${namespace}.${name} = | |
options = setAttrByPath path ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You find this clearer ?
There was a problem hiding this comment.
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
lib/plugin/mk-plugin.nix
Outdated
|
||
config = | ||
let | ||
cfg = config.${namespace}.${name}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cfg = config.${namespace}.${name}; | |
cfg = getAttrFromPath path config; |
No description provided.