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.

Sign up for GitHub

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

Jump to bottom

plugins: Add ccc (new PR) #1365

Merged
merged 4 commits into from
Apr 2, 2024
Merged

plugins: Add ccc (new PR) #1365

merged 4 commits into from
Apr 2, 2024

Conversation

jukremer
Copy link
Contributor

@jukremer jukremer commented Apr 1, 2024

No description provided.

@jukremer jukremer mentioned this pull request Apr 1, 2024
Closed
@jukremer
Copy link
Contributor Author

jukremer commented Apr 1, 2024 

None failed to evaluate:
error: derivation 'x86_64-linux.tests' does not have valid outputs: error:
              … while calling the 'getAttr' builtin

                at /derivation-internal.nix:19:19:

                  18|       value = commonAttrs // {
                  19|         outPath = builtins.getAttr outputName strict;
                    |                   ^
                  20|         drvPath = strict.drvPath;

              … while calling the 'derivationStrict' builtin

                at /derivation-internal.nix:9:12:

                   8|
                   9|   strict = derivationStrict drvAttrs;
                    |            ^
                  10|

              (stack trace truncated; use '--show-trace' to show the full trace)

              error: The option `plugins.ccc.default_color' does not exist. Definition values:
              - In `<unknown-file>': "#FFFFFF"

What does this error mean?

@MattSturgeon
Copy link
Member

MattSturgeon commented Apr 1, 2024 •
edited
Loading

The option `plugins.ccc.default_color' does not exist.

What does this error mean?

The test is malformed. Probably an issue with my suggestion.

You need to nest any options declared in settingsOptions in a settings block;

# e.g.
plugins.ccc.settings.default_color

@MattSturgeon
Copy link
Member

MattSturgeon commented Apr 1, 2024 •
edited
Loading

I think I need to create a new pull request. At least I don't see how I could append to this one

You would've been able to either push up more commits implementing the changes (the maintainers could then squash-merge them) or used one of many methods of amending history followed by a --force push.

History can be edited several ways, such as

  • git commit --amend to update the latest commit, git reset --soft <some ref>
  • git reset --mixed <some ref>, followed by making a new commit
  • git rebase --interactive <some ref> to do more advanced stuff

You can also use lazygit's interface to abstract a lot of that.

I'd highly recommend learning some of gits fundamentals and advanced features when you get chance.

@traxys
Copy link
Member

traxys commented Apr 1, 2024

Note that we can squash the commits when merging too

MattSturgeon
MattSturgeon requested changes Apr 1, 2024
View reviewed changes
Copy link
Member

@MattSturgeon MattSturgeon left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looking good. I've left some comments on the options and nit-picked their descriptions a little.

Bear in mind I'm not a maintainer here, so take my feedback with a pinch of salt!

plugins/utils/ccc.nix
lsp = false;
};
};
extraConfig = cfg: {opts.termguicolors = lib.mkDefault true;};
Copy link
Member

Choose a reason for hiding this comment

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

nit: I'd add a blank line above for consistency

plugins/utils/ccc.nix Outdated
'';

lsp = helpers.defaultNullOpts.mkBool true ''
Whether to enable nvim-lsp support. The color information is updated in the background and the result is used by `|:CccPick|` and highlighter.
Copy link
Member

Choose a reason for hiding this comment

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

Same comments as below

plugins/utils/ccc.nix Outdated
'';

lsp = helpers.defaultNullOpts.mkBool true ''
If true, highlight using nvim-lsp. If LS with the color provider is not attached to a buffer, it falls back to highlight with pickers. See also `|ccc-option-lsp|`.
Copy link
Member

Choose a reason for hiding this comment

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

  • nvim-lsp -> LSP
  • If LS -> If a language server
  • Description may be long enough to benefit from some line breaks?
  • Does it make sense to include vimdoc links here? Or should it just be "`:CccPick`"

plugins/utils/ccc.nix Outdated

highlighter = {
auto_enable = helpers.defaultNullOpts.mkBool false ''
Whether to enable automatically on BufEnter.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Wrap "BufEnter" in backtiicks

plugins/utils/ccc.nix Outdated

highlight_mode = helpers.defaultNullOpts.mkStr "bg" ''
Option to highlight text foreground or background. It is used to output_line and highlighter.
Options: `"fg" | "bg" | "foreground" | "background"`
Copy link
Member

Choose a reason for hiding this comment

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

This could be an enum if those are the only valid values.

Also same comments regarding vim doc links (see below)

plugins/utils/ccc.nix

settingsOptions = {
default_color = helpers.defaultNullOpts.mkStr "#000000" ''
The default color used when a color cannot be picked. It must be HEX format.
Copy link
Member

Choose a reason for hiding this comment

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

Should there be a warning/assertion that this is a valid hex string?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we ever check the validity of strings (apart from enumerations)

@jukremer
Copy link
Contributor Author

jukremer commented Apr 2, 2024

I changed the option to enum and addressed some of the comments on formatting. I wasn't sure how to format the text, went with 80 char max line width were it seemed appropriate. Is there a guideline for such styling?

@traxys
Copy link
Member

traxys commented Apr 2, 2024

I changed the option to enum and addressed some of the comments on formatting. I wasn't sure how to format the text, went with 80 char max line width were it seemed appropriate. Is there a guideline for such styling?

Not really. I use mostly 100 chars personally

@traxys traxys merged commit 7baefc8 into nix-community:main Apr 2, 2024
51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@traxys traxys traxys approved these changes

@MattSturgeon MattSturgeon MattSturgeon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jukremer @MattSturgeon @traxys