Nix docs: remove with lib; from example code
#293767
Conversation
AndersonTorres
requested review from
lilyinstarlight,
winterqt,
IvarWithoutBones,
kalbasit,
Mic92,
zowoq,
figsoda,
FRidh and
mweinelt
as code owners
github-actions
bot
added
6.topic: python
8.has: documentation
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/3587 |
delroth
added
the
12.approvals: 1
ofborg
bot
added
10.rebuild-darwin: 0
Following [Best Practices](https://nix.dev/guides/best-practices#with-scopes), `with` is a problematic language construction and should be avoided. Usually it is employed like a "factorization": `[ X.A X.B X.C X.D ]` is written `with X; [ A B C D ]`. However, as shown in the link above, the syntatical rules of `with` are not so intuitive, and this "distributive rule" is very selective, in the sense that `with X; [ A B C D ]` is not equivalent to `[ X.A X.B X.C X.D ]`. However, this factorization is still useful to "squeeze" some code, especially in lists like `meta.maintainers`. On the other hand, it becomes less justifiable in bigger scopes. This is especially true in cases like `with lib;` in the top of expression and in sets like `meta = with lib; { . . . }`. That being said, this patch removes most of example code in the current documentation. The exceptions are, for now - doc/functions/generators.section.md - doc/languages-frameworks/coq.section.md because, well, they are way more complicated, and I couldn't parse them mentally - yet another reason why `with` should be avoided!
delroth
removed
the
12.approvals: 1
| meta = with lib; { | ||
| meta = { | ||
| changelog = "https://github.com/pytest-dev/pytest/releases/tag/${version}"; | ||
| description = "Framework for writing tests"; | ||
| homepage = "https://github.com/pytest-dev/pytest"; | ||
| license = licenses.mit; | ||
| maintainers = with maintainers; [ domenkozar lovek323 madjar lsix ]; | ||
| license = lib.licenses.mit; | ||
| maintainers = with lib.maintainers; [ domenkozar lovek323 madjar lsix ]; | ||
| }; | ||
| } |
There was a problem hiding this comment.
Could you please elaborate the reasons?
There was a problem hiding this comment.
Because it makes meta simpler, and there is no danger of getting things from lib, when they are part of the build function. E.g. version will not be pulled from lib.
There was a problem hiding this comment.
Thanks now I understand your position.
I mostly agree, technically this is true. However, I saw many PRs where people were removing it from some files, especially NixOS modules. Having it on top of the file is definitely confusing and could potentially make the reasoning a bit harder in some cases.
I personally don't like it much, it's confusing and I never feel the need to use it... Only with lists sometimes, but it's rare.
Now, I don't judge what this PR is doing but I believe that using it with parsimony is fine.
IMHO, this PR is right for me, I don't really find a justification for using it, and therefore it should be removed.
There was a problem hiding this comment.
I believe that the idea of "distributive property of with" is strongly ingrained on the minds of many contributors around here.
I believe it is a compromise between the overzealous of removing with unconditionally on a side, and the overly globalized scope of with on the other.
Also, as said by eclairevoyant in #208242 (comment), the documentation encourages that reckless use of with lib;.
We should be consistent among the docs.
Also I believe that the arguments against restricting the scopes of with are not well grounded.
Why there are so many appearances of lib.mdDoc inside nixos/, at the same time they are under a with lib; scope?
I found more than 12k+ matches for lib.mdDoc, and more than 50 such appearances on systemd-unit-options.nix, why?
Treewide massive code generation is not an acceptable answer anymore since this phenomenon appears around new code (generally from novices that pick existing code as a template).
E.g. version will not be pulled from lib.
You don't really know this until someone evaluates with. And the rules of with are far from being intuitive
There was a problem hiding this comment.
I found more than 12k+ matches for lib.mdDoc, and more than 50 such appearances on systemd-unit-options.nix, why?
Likely because they were added for all descriptions during the docs migration.
There was a problem hiding this comment.
Likely because they were added for all descriptions during the docs migration.
And they continue to be added by novices and veterans alike - I bet a chocolate bar they don't know why lib.mdDoc is there since with lib; is already there.
There was a problem hiding this comment.
Also, as said by eclairevoyant in #208242 (comment), the documentation encourages that reckless use of
with lib;.
NB my comment was merely meant to point out that the docs were inconsistent with the tracking issue, and that inconsistency should be rectified, because I'd like to avoid future back-and-forth in PR reviews or otherwise avoidable PR churn.
Despite being a with-hater myself, I do not have a strong opinion on meta = with lib; in nixpkgs, other than avoiding that pattern seems less contentious than using it. (with lib; at the top of a file should obviously be discouraged.)
There was a problem hiding this comment.
@eclairevoyant thanks for expanding and explaining your point.
I didn't @-mark you because I didn't want to bother you with an old reference.
Apologies for not taking that context into account.
On the other hand my reasoning was justifiable: when the documentation includes examples of with lib;, then the documentation encourages its usage.
Following Best Practices,
withis a problematic language construction and should be avoided.Usually it is employed like a "factorization":
[ X.A X.B X.C X.D ]is writtenwith X; [ A B C D ].However, as shown in the link above, the syntatical rules of
withare not so intuitive, and this "distributive rule" is very selective, in the sense thatwith X; [ A B C D ]is not equivalent to[ X.A X.B X.C X.D ].However, this factorization is still useful to "squeeze" some code, especially in lists like
meta.maintainers.On the other hand, it becomes less justifiable in bigger scopes. This is especially true in cases like
with lib;in the top of expression and in sets likemeta = with lib; { . . . }.That being said, this patch removes most of example code in the current documentation.
The exceptions are, for now
because, well, they are way more complicated and I couldn't parse them mentally - another reason why
withshould be avoided!Description of changes
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.