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

highlight: init #571

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

LilleAila
Copy link
Contributor

Creates options for configuring highlight groups, mirroring the options available in nvim_set_hl.

@LilleAila LilleAila requested a review from NotAShelf as a code owner January 19, 2025 17:04
github-actions bot pushed a commit that referenced this pull request Jan 19, 2025
Copy link

github-actions bot commented Jan 19, 2025

🚀 Live preview deployed from c002e8f

View it here:

Debug Information

Triggered by: LilleAila

HEAD at: highlight-options

Reruns: 103

modules/neovim/init/highlight.nix Outdated Show resolved Hide resolved
modules/neovim/init/highlight.nix Outdated Show resolved Hide resolved
mkOption {
type = nullOr bool;
default = null;
description = ''Whether to enable ${name}'';
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
description = ''Whether to enable ${name}'';
description = "Whether to enable ${name}";

Also move example between default and description please.

modules/neovim/init/highlight.nix Outdated Show resolved Hide resolved
modules/neovim/init/highlight.nix Outdated Show resolved Hide resolved
)
cfg;
in
entryAnywhere (concatStringsSep "\n" highlights);
Copy link
Owner

Choose a reason for hiding this comment

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

entryAnywhere should be fine, but we might benefit more from placing this before plugin setup since some of them might actually use the highlights set in this module.

docs/release-notes/rl-0.8.md Outdated Show resolved Hide resolved
@LilleAila LilleAila requested a review from NotAShelf January 19, 2025 18:39
Comment on lines +14 to +30
mkColorOption = target:
mkOption {
type = nullOr hexColor;
default = null;
example = "#ebdbb2";
description = ''
The ${target} color to use. Written as color name or hex "#RRGGBB".
'';
};

mkBoolOption = name:
mkOption {
type = nullOr bool;
default = null;
example = false;
description = "Whether to enable ${name}";
};
Copy link
Owner

Choose a reason for hiding this comment

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

Not a blocker, but these may be better suited for the extended lib.

Comment on lines +46 to +56
bold = mkBoolOption "bold";
standout = mkBoolOption "standout";
underline = mkBoolOption "underline";
undercurl = mkBoolOption "undercurl";
underdouble = mkBoolOption "underdouble";
underdotted = mkBoolOption "underdotted";
underdashed = mkBoolOption "underdashed";
strikethrough = mkBoolOption "strikethrough";
italic = mkBoolOption "italic";
reverse = mkBoolOption "reverse";
nocombine = mkBoolOption "nocombine";
Copy link
Owner

Choose a reason for hiding this comment

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

mapAttrs (_: mkBoolOption) can probably handle this with less repetition of code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

        // builtins.listToAttrs (map (option:
          nameValuePair option (mkOption {
            type = nullOr bool;
            default = null;
            example = false;
            description = "Whether to enable ${option}";
          })) [
          "bold"
          "standout"
          "underline"
          "undercurl"
          "underdotted"
          "underdashed"
          "strikethrough"
          "italic"
          "reverse"
          "nocombine"
          "force"
        ]);

Something like this? I don't really like that code, i think it looked better with repetition of mkBoolOption

Copy link
Owner

Choose a reason for hiding this comment

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

mapAttrs (_: mkBoolOption) {
  bold = "bold";
  standout = "standout";
  underline = "underline";
  undercurl =  "undercurl";
  underdouble = "underdouble";
  underdotted = "underdotted";
  underdashed = "underdashed";
  strikethrough =  "strikethrough";
  italic = "italic";
  reverse =  "reverse";
  nocombine =  "nocombine";
}

would do it if I'm not mistaken. But you can keep the current method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel that the code looks better with the repetition, and the two alternatives only introduce unneeded complexity and makes the code harder to read, even when there is slightly more repetition.

modules/neovim/init/highlight.nix Outdated Show resolved Hide resolved
modules/neovim/init/highlight.nix Outdated Show resolved Hide resolved
)
cfg;
in
entryBetween ["lazyConfigs" "pluginConfigs" "extraPluginConfigs"] ["theme"] (concatLines highlights);
Copy link
Owner

Choose a reason for hiding this comment

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

CC @horriblename on this. I'm not actually sure where lazyConfigs is located in the DAG system.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wrote it based on the code in modules/wrapper/rc/config.nix, where those three are listed as after each other.

github-actions bot pushed a commit that referenced this pull request Jan 20, 2025
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