-
Notifications
You must be signed in to change notification settings - Fork 2
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
Stylistic Nix expression refactoring #3
Conversation
First of all, thanks for taking the time to make the Nix code better :) However, I don't know what to do with the PR. I understand that the code is much better, but on the other hand it's much longer, and I don't think that having more intermediate variables adds to much to the understanding. Do you think it could be possible for you to have a PR in which this is written as an |
6162d3d
to
e6aca84
Compare
I'd prefer not to put this in an I've gone through and shortened the changeset a bit, removing some of the intermediate variables and collapsing the separate overlays down into a single set of overrides. Additionally, I introduced At the very least, even if the rest of these changes don't make it in, I think the original Nix expression should be updated to avoid mixing mixing |
e6aca84
to
3644ea9
Compare
sha256 = "0q44lxzz8pp89ccaiw3iwczha8x2rxjwmgzkxj8cxm97ymsm0diy"; | ||
} | ||
) {} | ||
).pkgs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cf. https://nix.dev/anti-patterns/language.html#with-attrset-expression as the reason for why I'm avoiding the more concise with
syntax that's used in the original default.nix
.
@jkachmar thanks for going the extra step. I think that know the file looks much better, and I honestly I wasn't even aware that I was using an anti-pattern. |
This is very much a subjective/stylistic PR, so feel free to request significant changes or even close it entirely if you feel that it oversteps your intentions for this repo.
I saw this posted on r/haskell and figured that since it’s a template, it might end up in front of people who aren’t necessarily super familiar with Nix.
To that end, I’ve refactored the
default.nix
expression to lay things out in a way that I think might make it easier for others to grow and extend parts of the expression gracefully.In particular, I’ve added: