Skip to content

stylix: cleanup mkTarget impl #1512

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

Closed
0xda157 wants to merge 1 commit into from
Closed

stylix: cleanup mkTarget impl #1512

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 27 additions & 27 deletions stylix/mk-target.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ let
builtins.attrNames
];

getStylixAttrs =
getArgs =
fn:
lib.genAttrs (functionArgNames fn) (
arg:
Expand All @@ -222,26 +222,27 @@ let
);

# Call the configuration function with its required Stylix arguments.
mkConfig = fn: fn (getStylixAttrs fn);
applyArgs = fn: fn (getArgs fn);
Copy link
Copy Markdown
Member

@MattSturgeon MattSturgeon Jun 17, 2025

Choose a reason for hiding this comment

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

Do we actually need a function to apply getArgs to a function, when we already have a function to easily get the args?

I.e. is applyArgs foo actually better than foo (getArgs foo)?

I ask, because I'm wondering if it'd be more useful to have a maybeApply, used like maybeApply foo (getArgs foo):

maybeApply =
  maybeFn: args:
  if isFunction maybeFn then maybeFn args else maybeFn;

But I think the current applyArgs would break that abstraction 🤔

Copy link
Copy Markdown
Member

@trueNAHO trueNAHO Jun 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Do we actually need a function to apply getArgs to a function, when we already have a function to easily get the args?

I.e. is applyArgs foo actually better than foo (getArgs foo)?

With the previous mkConfig terminology, it seemed obvious to provide this wrapper. Now, it would simplify the code by inlining this applyArgs function.

I ask, because I'm wondering if it'd be more useful to have a maybeApply, used like maybeApply foo (getArgs foo):

maybeApply =
  maybeFn: args:
  if isFunction maybeFn then maybeFn args else maybeFn;

But I think the current applyArgs would break that abstraction 🤔

I think it is better to keep the explicit if builtins.isFunction c then conditionalConfig c else c case in the main loop, as currently done.


# Safeguard configuration functions when any of their arguments is
# disabled.
mkConditionalConfig =
c:
if builtins.isFunction c then
let
allAttrsNonNull = lib.pipe c [
getStylixAttrs
builtins.attrValues
(builtins.all (attr: attr != null))
];
in
lib.mkIf allAttrsNonNull (mkConfig c)
else
c;
# disabled, while non-function configurations are unguarded.
conditionalConfig =
f:
let
allAttrsNonNull = lib.pipe f [
getArgs
builtins.attrValues
(builtins.all (attr: attr != null))
];
in
lib.mkIf allAttrsNonNull (applyArgs f);

applyIfFn = f: v: if builtins.isFunction v then f v else v;
in
{
inherit imports;
imports = imports ++ [
{ options.stylix.targets.${name} = extraOptions; }
];

options.stylix.targets.${name}.enable =
let
Expand All @@ -261,20 +262,19 @@ let
config = lib.mkIf (config.stylix.enable && cfg.enable) (
lib.mkMerge (
lib.optional (generalConfig != null) (
mkConfig (
if builtins.isPath generalConfig then import generalConfig else generalConfig
)
)
++ map (c: mkConditionalConfig (if builtins.isPath c then import c else c)) (
lib.toList configElements
let
c =
if builtins.isPath generalConfig then import generalConfig else generalConfig;
in
applyIfFn applyArgs c
)
++ map (
c: applyIfFn conditionalConfig (if builtins.isPath c then import c else c)
) (lib.toList configElements)
)
);
};
in
{
imports = [
{ options.stylix.targets.${name} = extraOptions; }
module
];
imports = [ module ];
}