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

tt 0 11 wip #47

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

tt 0 11 wip #47

wants to merge 2 commits into from

Conversation

Misterio77
Copy link
Owner

@Misterio77 Misterio77 commented Feb 13, 2024

#44

variant = set.variant or null;

# Colors
palette = set.palette or set.colors or (lib.filterAttrs (n: _: lib.hasPrefix "base" n) set);
Copy link
Owner Author

Choose a reason for hiding this comment

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

To keep backwards compat with top-level base* keys

validChars = lib.stringToCharacters "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz1234567890-";
isValid = x: lib.elem x validChars;

slugify = str: lib.toLower (filterChars isValid (lib.replaceStrings [" "] ["-"] str));
Copy link
Owner Author

Choose a reason for hiding this comment

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

The spec instructs us to instead replace all non-ASCII with their ASCII approximations, and only then drop the non alphanum ones.

ASCII approximations is apperently very tricky with pure nix. Lookup tables seems to be the way to go, but it's not an ideal solution performance wise either.

https://github.com/tinted-theming/home/blob/main/builder.md#slugify

Comment on lines -29 to +46
type = types.str;
default = "";
type = types.nullOr types.str;
default = null;
example = "Awesome Scheme";
description = ''
Color scheme (pretty) name
'';
};
description = mkOption {
type = types.str;
default = "";
type = types.nullOr types.str;
default = null;
example = "A very nice theme";
description = ''
Color scheme author
'';
};
author = mkOption {
type = types.str;
default = "";
type = types.nullOr types.str;
default = null;
Copy link
Owner Author

Choose a reason for hiding this comment

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

This probably is a breaking change for people defining their schemes in straight nix and that decided to omit name/description/author. People using schemes provided by schemeFromYAML should not come across null values, though.

I think it's more correct to default to null and let consumers handle what to do with null values instead. I wonder if there's a good way to communicate this change with a warning or similar.

Copy link
Owner Author

Choose a reason for hiding this comment

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

The slug has always been required in nix-colors, but maybe I should instead follow the spec more closely and make name required and slug optional (inferrable by slugify).

This might be a breaking change, unless:

  • Do some conditionals to default name to slug if it's non-null; and/or
  • Set a fallback generic name with a warning

@Misterio77
Copy link
Owner Author

Misterio77 commented Feb 13, 2024

As mentioned in #44 (comment):

The main problem is that there isn't really a pure nix yaml converter available. The one we're using is hackish and does not work with more complex (e.g. non-top level attributes) yaml, which the 0.11 spec now uses.

There's a few possibilities here:

  • Use IFD
    • Affects performance significantly
    • Requires changing our current colorSchemes.<scheme> interface to colorSchemes.<system>.<scheme>.
  • Pre-process and vendor JSON file(s) with the schemes
    • This was my approach before 2.0:
    • Will break the current feature that allows users to change the inputs.follows to switch nix-colors to different upstreams
  • Wait for Add support for parsing YAML NixOS/nix#7340
    • Even if merged, will be an experimental feature for a while, so not ideal either
  • Ask tinted theming to include JSON versions of the schemes (?)
  • Somehow try to make the current fromYaml function work with nested keys

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.

1 participant