Add mkEnableTargetWith and use it for documenting dynamic autoEnable conditions#1244
Conversation
6f5af4e to
dcb83df
Compare
dcb83df to
314d34f
Compare
314d34f to
4559ec9
Compare
6d0e88a to
bf825ef
Compare
mkEnableTargetWith and use if for documenting dynamic autoEnable conditionsmkEnableTargetWith and use it for documenting dynamic autoEnable conditions
bf825ef to
deed6ac
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
deed6ac to
9422acd
Compare
This comment was marked as resolved.
This comment was marked as resolved.
9422acd to
27b8efe
Compare
trueNAHO
left a comment
There was a problem hiding this comment.
The implementation looks very nice :)
Should we merge this patchset with the following merge commit message:
treewide: add and apply mkEnableIf and mkEnableTargetWith functions
Link: https://github.com/nix-community/stylix/pull/1244
Reviewed-by: awwpotato <awwpotato@voidq.com>
Reviewed-by: NAHO <90870942+trueNAHO@users.noreply.github.com>
c7bbd97 to
6f81d45
Compare
48f21d3 to
2c37613
Compare
trueNAHO
left a comment
There was a problem hiding this comment.
it's getting frustrating to keep rebasing this, would it be possible to get a final review?
Sorry for letting this PR pend so long.
Either way, unless this patchset depends on new commits or this patchset would substantially change with the new commits, continuously running git rebase master is technically not required while the patchset itself is being discussed. A simpler approach would be to resolve the current state and git rebase master once at the very end just before merging the patchset.
Continuously running git rebase master does have the benefit that the patchset can be directly merged without requesting another git rebase master. This is a trade-off between being merge-ready and going insane with git rebase master. Although successfully completing git rebase master is its own reward.
6c1881c to
a52fb60
Compare
There was a problem hiding this comment.
Looks great, except for one small question.
I would adapt my previous merge commit message (not squash commit message) proposal to the following:
treewide: use mkEnableTargetWith for dynamic autoEnable conditions
Use the new mkEnableTargetWith function for dynamic autoEnable
conditions to prevent the evaluation of dynamic conditions during
documentation rendering.
The 'same as `stylix.autoEnable`' defaultText is replaced with
'stylix.autoEnable' or 'stylix.autoEnable && ${autoEnableExpr}'
depending on whether autoEnableExpr is provided. For readability,
autoEnable expressions containing Nix operators of lower precedence than
'&&' are automatically wrapped in parentheses, unless autoWrapExpr is
disabled.
Closes: https://github.com/nix-community/stylix/issues/98
Link: https://github.com/nix-community/stylix/pull/1244
Reviewed-by: awwpotato <awwpotato@voidq.com>
Reviewed-by: NAHO <90870942+trueNAHO@users.noreply.github.com>
Reviewed-by: Daniel Thwaites <danth@danth.me>
Let's optimistically close #98 with this patchset. If this issue is in fact not resolved, we can simply re-open it.
Drop the redundant "same as" prefix and instead use a `literalExpression`. Co-authored-by: NAHO <90870942+trueNAHO@users.noreply.github.com>
Refactor `mkEnableTarget` and `mkEnableWallpaper` to use `mkEnableIf`.
This allows correctly documenting dynamic enable conditions.
E.g:
```nix
enable = mkEnableTargetWith {
name = "QT";
autoEnable = pkgs.stdenv.hostPlatform.isLinux;
autoEnableExpr = "pkgs.stdenv.hostPlatform.isLinux";
};
```
`autoEnableExpr` will be wrapped in parentheses if it contains a nix
operator with lower precedence than `&&`.
Added a note in the docs mentioning `mkEnableTarget` should either have
a static `autoEnable` or specify `autoEnableExpr`.
Ensure enable options with dynamic `autoEnable` are documented correctly.
234b987 to
289356d
Compare
SGTM
SGTM. There are likely other cases of unintentionally dynamic docs values this PR hasn't caught, since it doesn't introduce any enforcement. But let's be optimistic for now! |
danth
left a comment
There was a problem hiding this comment.
Latest revision looks good to me. Thanks for working on this :))
|
Successfully created backport PR for |
#1244) Use the new `mkEnableTargetWith` function for dynamic `autoEnable` conditions to prevent the evaluation of dynamic conditions during documentation rendering. The "same as `stylix.autoEnable`" `defaultText` is replaced with `stylix.autoEnable` or `stylix.autoEnable && ${autoEnableExpr}`, depending on whether `autoEnableExpr` is provided. For readability, `autoEnable` expressions containing Nix operators of lower precedence than `&&` are automatically wrapped in parentheses, unless `autoWrapExpr` is disabled. Closes: #98 Link: #1244 Reviewed-by: awwpotato <awwpotato@voidq.com> Reviewed-by: NAHO <90870942+trueNAHO@users.noreply.github.com> Reviewed-by: Daniel Thwaites <danth@danth.me>
Note
This PR is comprised of intentionally separate commits, please don't squash 🙂
This is a solution to the issues uncovered by #1212. This should fix #98 (not tested).
Previously, the literal markdown
same as `stylix.autoEnable`was used. When an additionalconditionTextis supplied, it will now use the literal expressionstylix.autoEnable && ${expr}.If the
exprcontains any nix operators with a lower priority than&&, it will be auto-wrapped in parentheses, unlessautoWrapExpr = falseis used.We could stick with the previous markdown approach, but I'm unsure what the best way to approach merging it with a
autoEnableExprorautoEnableTextorautoEnableMDstyle arg would be. Suggestions and/or followup PRs welcome.Things done
Notify relevant people
mkTarget