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

treewide: propagate inputs and remove templates #926

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Flameopathic
Copy link
Contributor

fixes #238

@Flameopathic Flameopathic force-pushed the propagate-inputs branch 4 times, most recently from f9af7a5 to 60fe812 Compare February 26, 2025 22:18
@Flameopathic
Copy link
Contributor Author

inputs does not seem to be properly propagating; it's likely that i'm somehow setting it wrong.

@Flameopathic Flameopathic marked this pull request as ready for review February 27, 2025 21:03
Copy link
Owner

@danth danth left a comment

Choose a reason for hiding this comment

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

I'm fairly sure this will cause a conflict if the user already has their own inputs in _module.args (or specialArgs). It's pretty common for people to do this.

I suggested an alternative implementation in #873 (comment): essentially, give each module a stylix.targets.*.template option and set these options from flake.nix, as we're now doing for some of the other flake inputs:

stylix/flake.nix

Lines 221 to 225 in c74352a

stylix = {
paletteGenerator = self.packages.${pkgs.system}.palette-generator;
base16 = base16.lib args;
homeManagerIntegration.module = self.homeManagerModules.stylix;
};

This still requires listing each template, however it's cleaner than passing inputs through as a function argument.

@Flameopathic
Copy link
Contributor Author

I'm fairly sure this will cause a conflict if the user already has their own inputs in _module.args (or specialArgs).

crap, you're probably right

I suggested an alternative implementation in #873 (comment): essentially, give each module a stylix.targets.*.template option and set these options from flake.nix, as we're now doing for some of the other flake inputs: [...]

that would work, though it seems like it wouldn't scale as well as giving the modules access to inputs, and i still have hope for there being a clean way of doing it.

if you have a strong enough preference, i'll go straight for the stylix.targets.*.template option; otherwise i think giving all modules inputs would result in the best developer experience

@danth
Copy link
Owner

danth commented Feb 28, 2025

Alternatively, we could just use a different name under _module.args.

Or have just one readonly option containing all the inputs.

I was also thinking with the stylix.targets.*.template options that it might be possible to make these public, to give users an easier way to override the template if they want to. That does raise backwards compatibility issues if we want to replace the template with inline options in the future, though.

@Flameopathic
Copy link
Contributor Author

Alternatively, we could just use a different name under _module.args.

Or have just one readonly option containing all the inputs.

of these two, the latter would probably be the most intuitive to contributors. i suppose that's what i'm leaning toward ATP

I was also thinking with the stylix.targets.*.template options that it might be possible to make these public, to give users an easier way to override the template if they want to. That does raise backwards compatibility issues if we want to replace the template with inline options in the future, though.

that could be nice, but would users be able to do anything other than override definitions of single colors? i think something like that would be better handled by #685

@trueNAHO
Copy link
Collaborator

Alternatively, we could just use a different name under _module.args.
Or have just one readonly option containing all the inputs.

of these two, the latter would probably be the most intuitive to contributors.

Agreed.

I was also thinking with the stylix.targets.*.template options that it might be possible to make these public, to give users an easier way to override the template if they want to. That does raise backwards compatibility issues if we want to replace the template with inline options in the future, though.

that could be nice, but would users be able to do anything other than override definitions of single colors? i think something like that would be better handled by #685

Agreed.

@Flameopathic Flameopathic marked this pull request as draft February 28, 2025 19:29
@Flameopathic
Copy link
Contributor Author

Flameopathic commented Feb 28, 2025

if making a read-only option, would stylix.inputs or lib.stylix.inputs be better? what exactly should go in lib vs not?

also how are options being put under lib? i can't seem to find any option definitions starting with lib.stylix, though i could be missing something because there are many, many usages of lib.

@Flameopathic
Copy link
Contributor Author

current solution makes read-ony option stylix.inputs and populates it with all inputs from the Stylix flake

@Flameopathic Flameopathic marked this pull request as ready for review March 1, 2025 19:36
@danth
Copy link
Owner

danth commented Mar 3, 2025

lib is a special option which allows anything to be stored inside it without any type checking.

We're moving away from this to use defined read-only options instead.

Copy link
Owner

@danth danth left a comment

Choose a reason for hiding this comment

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

We should mark the option as internal = true since it's not meant to be used externally. This hides it from the public documentation.

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.

stylix: remove separate list of template files
3 participants