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

plugins/nvim-highlight-colors: init #2105

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Tom-Hubrecht
Copy link

No description provided.

@Tom-Hubrecht
Copy link
Author

This adds a plugin for nvim-highlight-colors, it is a draft as the corresponding vimPlugin does not exist in nixpkgs and I do not intend to contribute it upstream.

@refaelsh
Copy link
Contributor

refaelsh commented Sep 3, 2024

the corresponding vimPlugin does not exist in nixpkgs

In that case, is that even possible to add such a plugin to nixvim?

@refaelsh
Copy link
Contributor

refaelsh commented Sep 3, 2024

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.

The CI failure seems to just be a typo. Otherwise this is more or less there, suggestions below are fairly minor. 😁

@@ -0,0 +1,96 @@
{
lib,
helpers,
Copy link
Member

Choose a reason for hiding this comment

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

helpers is deprecated, you can use lib.nixvim instead:

Suggested change
helpers,

Comment on lines +10 to +16
inherit (helpers.defaultNullOpts)
mkBool
mkEnum
mkListOf
mkStr
mkStr'
;
Copy link
Member

Choose a reason for hiding this comment

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

To match the style used elsewhere in nixvim:

Suggested change
inherit (helpers.defaultNullOpts)
mkBool
mkEnum
mkListOf
mkStr
mkStr'
;
inherit (lib.nixvim) defaultNullOpts;

mkStr'
;
in
helpers.neovim-plugin.mkNeovimPlugin config {
Copy link
Member

Choose a reason for hiding this comment

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

We (very) recently dropped the config arg:

Suggested change
helpers.neovim-plugin.mkNeovimPlugin config {
lib.nixvim.neovim-plugin.mkNeovimPlugin {

in
helpers.neovim-plugin.mkNeovimPlugin config {
name = "nvim-highlight-colors";
defaultPackage = pkgs.vimPlugins.nvim-highlight-colors;
Copy link
Member

Choose a reason for hiding this comment

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

Watch out for #2139, if it is merged first you'll have to do:

Suggested change
defaultPackage = pkgs.vimPlugins.nvim-highlight-colors;
package = "nvim-highlight-colors";

name = "nvim-highlight-colors";
defaultPackage = pkgs.vimPlugins.nvim-highlight-colors;

maintainers = [ helpers.maintainers.thubrecht ];
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
maintainers = [ helpers.maintainers.thubrecht ];
maintainers = [ lib.maintainers.thubrecht ];

Copy link
Member

Choose a reason for hiding this comment

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

Could you introduce a test file please, you can reference other tests in tests/test-sources/plugins and/or recently merged PRs.

Generally, we like to have an empty test case, a defaults test case and (ideally) an example test case.


maintainers = [ helpers.maintainers.thubrecht ];

settingsOptions = {
Copy link
Member

Choose a reason for hiding this comment

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

Note: because settings is a freeform option, there is no need to declare sub-options for every upstream plugin option. Users can define any config they like, so having too many options can just end up being a maintenance burden.

The judgement call is yours to make though, I won't block a PR for having "too many" settings options 😁

Comment on lines +25 to +29
render = mkEnum [
"background"
"foreground"
"virtual"
] "background" "The render style used.";
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
render = mkEnum [
"background"
"foreground"
"virtual"
] "background" "The render style used.";
render = defaultNullOpts.mkEnumFirstDefault [
"background"
"foreground"
"virtual"
] "The render style used.";

Comment on lines +79 to +80
exclude_filetypes = mkListOf lib.stypes.str [ ] "A list of filetypes to exclude from highlighting.";
exclude_buftypes = mkListOf lib.stypes.str [ ] "A list of buftypes to exclude from highlighting.";
Copy link
Member

Choose a reason for hiding this comment

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

Typo:

Suggested change
exclude_filetypes = mkListOf lib.stypes.str [ ] "A list of filetypes to exclude from highlighting.";
exclude_buftypes = mkListOf lib.stypes.str [ ] "A list of buftypes to exclude from highlighting.";
exclude_filetypes = mkListOf lib.types.str [ ] "A list of filetypes to exclude from highlighting.";
exclude_buftypes = mkListOf lib.types.str [ ] "A list of buftypes to exclude from highlighting.";

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.

3 participants