-
-
Notifications
You must be signed in to change notification settings - Fork 300
firefox: move options into profiles submodule #2073
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
base: master
Are you sure you want to change the base?
Conversation
58ef0d1 to
7195791
Compare
7195791 to
9e5c499
Compare
1289331 to
bc78620
Compare
|
I'm currently stuck on the below error, which doesn't make any sense: |
This error is typically caused by https://nixos.org/manual/nixos/unstable/#sec-option-types-submodule When options = { … };the module system believes you are defining the option: which doesn’t exist, leading to the error you’re seeing. A common workaround is to use a module form that isn’t a plain attribute set, e.g.:
which disable the shorthand behavior, or to use |
|
Although, if that's a home-manager configuration, I don't think it uses Is Home Manager's NixOS module imported into the |
stylix/mk-target.nix
Outdated
| `submodule` (Module -> Module) | ||
| : A function which transforms a module into a submodule, for example: | ||
| ```nix | ||
| module: { | ||
| options.programs.firefox.profiles = lib.mkOption { | ||
| type = lib.types.attrsOf (lib.types.submodule module); | ||
| }; | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not really read the diff, but is there no way around extending the Stylix module system by using existing functionality? It would be sad to increase its complexity unless absolutely necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not really read the diff, but is there no way around extending the Stylix module system by using existing functionality? It would be sad to increase its complexity unless absolutely necessary.
I have an alternative solution which should make the diff on /stylix/mk-target.nix very small, but I don't have time to implement it this week.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've implemented the alternative solution and it basically doesn't work. mkTarget need to access to config from the top-level so as far as I can tell the only way to do it is by passing some submodule function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After backtracking conversations, I do not fully understand what is going on and can therefore not really help. Specifically, I do not understand how the end-user interface is supposed to change and how this should be implemented.
IIUC, the issue is that extending profile declarations with Stylix theming currently requires explicitly declaring profiles to be extended such that the following application is possible:
stylix.targets.${target}.profileNames = ["one" "two" "three"];
programs.${program}.profiles =
lib.genAttrs config.stylix.targets.${target}.profileNames /* ... */;-
Why can we not simply extend all submodules by iterating over them, or does this lead to infinite recursion?
-
IIUC, the proposal is also about adding top-level
stylix.${target}.profiles.${profile}.enableoptions to potentially disable theming for certain profiles. If the already defined Home Manager submodule can be extended, would it not be better to add this option understylix.targets.${target}.profiles.${profile}.enableorprograms.${program}.profiles.${profile}.stylix.enable?
Considering all of this, why does mkTarget need to interface a "submodule"? If this is just to declare options outside the default stylix.targets.${target} namespace, then this should already be possible by declaring them inside imports.
bc78620 to
0d53b78
Compare
Uh oh!
There was an error while loading. Please reload this page.