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

palette: fix eval fail when image is null #912

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

palette: fix eval fail when image is null #912

wants to merge 1 commit into from
+6 −5

Conversation

merrkry
Copy link

@merrkry merrkry commented Feb 24, 2025

Fixes #911

Allows stylix.base16Scheme to be set to null when stylix.image is null. Therefore the assertion in the module can be triggered as expected.

Also adds lib.mkIf to this internal module, as the mentioned assertion will cause eval fail when the whole stylix module is not enabled. E.g. on large config with multiple hosts, where this module is not enabled on some devices.

danth
danth requested changes Feb 24, 2025
View reviewed changes
@trueNAHO
Copy link
Collaborator

Cross-post:

Why is this not caught by the following assertion:

stylix/stylix/palette.nix

Lines 162 to 167 in 59d2c0e

assertions = [
{
assertion = !(cfg.image == null && cfg.base16Scheme == null);
message = "One of `stylix.image` or `stylix.base16Scheme` must be set";
}
];

base16Scheme has non-null default values. When nix tries to access its value in this assertion, the failure happens.

The cleanest solution might be to fix the assertion.

-- #911 (comment)

@merrkry
Copy link
Author

merrkry commented Feb 24, 2025

Newer version:

  • Wrap only the assertion with lib.mkIf as there are many use cases when stylix disabled
  • Only assert stylix.base16Schme. As image is optional now, we only need to make sure a color scheme is generated (default value) or explicitly set.

Flameopathic
Flameopathic approved these changes Feb 24, 2025
View reviewed changes
Copy link
Contributor

@Flameopathic Flameopathic left a comment

Choose a reason for hiding this comment

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

LGTM

@merrkry merrkry requested a review from danth February 24, 2025 20:50
@Flameopathic Flameopathic mentioned this pull request Feb 25, 2025
Open
trueNAHO
trueNAHO approved these changes Feb 26, 2025
View reviewed changes
Copy link
Collaborator

@trueNAHO trueNAHO left a comment

Choose a reason for hiding this comment

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

LGTM.

@merrkry
Copy link
Author

merrkry commented Feb 28, 2025

@danth It seems that your requested changes blocked the merge. Can you take a further look to clarify what's the to be discussed/improved on?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Flameopathic Flameopathic Flameopathic approved these changes

@trueNAHO trueNAHO trueNAHO approved these changes

@danth danth Awaiting requested review from danth

Requested changes must be addressed to merge this pull request.

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

Successfully merging this pull request may close these issues.

[Bug] Eval fail when stylix.image is null and stylix.base16Scheme is unset.
4 participants
@merrkry @trueNAHO @danth @Flameopathic