stylix: cleanup mkTarget impl#1512
Conversation
50cf9cd to
1c0c7ea
Compare
Flameopathic
left a comment
There was a problem hiding this comment.
I'm not sure that the function naming is perfect, but honestly I don't think it ever will be. Code looks nicer now, though! Thanks for the cleanup.
|
|
||
| # Call the configuration function with its required Stylix arguments. | ||
| mkConfig = fn: fn (getStylixAttrs fn); | ||
| applyArgs = fn: fn (getArgs fn); |
There was a problem hiding this comment.
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 🤔
There was a problem hiding this comment.
Do we actually need a function to apply
getArgsto a function, when we already have a function to easily get the args?I.e. is
applyArgs fooactually better thanfoo (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 likemaybeApply foo (getArgs foo):maybeApply = maybeFn: args: if isFunction maybeFn then maybeFn args else maybeFn;But I think the current
applyArgswould 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.
1c0c7ea to
4a2caa2
Compare
4a2caa2 to
2ffa39c
Compare
|
Superseded-by: #1721 |
inspired by #1368 (comment)
CC @MattSturgeon @Flameopathic @trueNAHO
Things done
Notify maintainers