-
-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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
Documentation: guide for using let in
vs rec
vs finalAttrs
#315337
Comments
At the very least I'd like reviewers to stop nitpicking between these. Unless there is explicit community-wide consensus (i.e. RFC), wide-spread historical precedent (i.e. mkDerivation attr order) or significant concrete problems with the code (i.e. correctness, readability), people should be free to use whichever code patterns or organisation they like. And by significant, I really do mean significant. Code being organised against your personal preference or patterns you dislike being used are minor differences in taste, not a significant readability problems. Nitpicking such "issues" is not in any way productive and utterly confusing/unwelcoming to newcomers. They are faced with dozens such implicit rules that aren't explicitly written down anywhere and, even worse, differ wildly depending on which person reviews the code. I don't doubt we've lost more than a dozen contributors whose first PR was faced with 12 nitpicks and 0 actually substantial review comments and then left to bitrot for months. I've been guilty of this aswell and we all just need to stop doing it. |
I was (and still am, I guess) one of those nitpickers and I think what really needs to happen here is to standardize the It almost feels pedantic, in a way, with the rule being more of a dogma, and the rationale behind it not being at all clear to new contributors and the applicability of it still very limited. We should really try to explain things more, come to a common standard, and make that standard applicable to everything. Perhaps a table could serve as a brief explanation as to why one pattern may be preferred over the other?
Also, the existence of let
withRec = stdenv.mkDerivation rec {
pname = "with-rec";
version = "0.1.0";
src = fetchFromGitHub {
owner = "example";
repo = "example";
rev = "v${version}";
hash = "...";
};
};
withFinalAttrs = stdenv.mkDerivation (finalAttrs: {
pname = "with-rec";
version = "0.1.0";
src = fetchFromGitHub {
owner = "example";
repo = "example";
rev = "v${finalAttrs.version}";
hash = "...";
};
});
in
lib.trace (withRec.overrideAttrs { version = "0.2.0"; }).src.rev # v0.1.0
lib.trace (withFinalAttrs.overrideAttrs { version = "0.2.0"; }).src.rev # v0.2.0
{};
`` |
i think #310373 is important to bring up in this conversation. at first glance, lib.trace (withRec.overrideAttrs { version = "0.2.0"; }).src.rev # v0.1.0
lib.trace (withFinalAttrs.overrideAttrs { version = "0.2.0"; }).src.rev # v0.2.0 while the
this is probably what i want most out of this issue, though. a lot of times the differences between these patterns are exactly that: nitpicks. i'm sure it's frustrated many here time and time again - and if it doesn't actually benefit our user in many cases, why are we doing it? why are we giving new contributors a hard time over preferred patterns to reach the same goal? i would personally encourage package maintainers to use their personal preference unless it's actually affecting how the expression is used or defined. for other readability/consistency concerns, we will hopefully have |
The problem with |
@getchoo made the same points I was going to make. To that I want to add that whenever this comes up, one should think about which parts of the package they want to expose as an external API and what parts you don't.
As a more robust alternative, I've recently discovered that you can simply make such parameters part of the regular Regardless of opinions on these manners though, a table like @pluiedev's would be great to have in the contributor's manual. We generally need more such short summaries and general guidelines to be written down in accessible places. I also want to mention that, in my book, it is perfectly valid to use |
This issue has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/more-official-guidelines-in-regards-to-styling-choices/48407/3 |
What's missing in this table is the performance overhead of finalAttrs. Also I probably should write proper benchmark for this, a performance decrease have been observed by several people because of the fixed input computation. |
btw, |
It's kinda part of the problem, isn't it? It's very inconsistent as to which builders have the fixed-point capability... |
Someone has to care enough to add it to the other builders, basically. Or redo the entire fixed-point concept from scratch. |
Problem
I often see maintainers and committers alike suggesting to use the
let in
syntax following the practices suggested by https://nix.dev/guides/best-practices.html. However, this is hardly uniform some would suggest usingrec
, and it's hard to get a solid grasp on what to use in each situation. This was only exemplified whenfinalAttrs
started popping up more commonly as of recent.Proposal
From my understanding https://nix.dev is an official source so perhaps it would be best to get everyone on board and aware of this best practice? Or maybe to get a more concrete set of best practices to use in each given situation. Or at the very least make members aware that the
let in
syntax is said to be the best practice, as stated in https://nix.dev/guides/best-practices.html.Checklist
Add a 👍 reaction to issues you find important.
The text was updated successfully, but these errors were encountered: