-
-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
Treewide Nix reformat pass 1 [skip treewide] #322537
base: master
Are you sure you want to change the base?
Conversation
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/formatting-team-meeting-2024-06-25/47661/1 |
I'd say we should block on NixOS/nixfmt#153. There are also many issues about either awkward formatting result or implementation correctness. It's no good to race reformatting everything before that.
This sounds too long to me. If the whole-repo checking takes too long, it blocks auto-checking in git commit hook. |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/tweag-nix-dev-update-57/47823/1 |
@oxalica The reason I'm pushing for this is because there's a lot of churn from people reformatting individual parts on their own, @mweinelt can attest to that. Let's not let perfect be the enemy of good, the formatter works and is unlikely to change drastically. We should definitely block on correctness bugs like NixOS/nixfmt#197 though, this is being worked on :) Regarding the time it takes for a full format, the solution I'd propose is to use a pre-commit hook that only formats files that were changed. This also nicely aligns with CI only requiring changed files to be formatted. I'd be okay with blocking on that for this PR, I'll add it to the TODO items. In addition to that, if CI fails, it shows the Alternative: treefmtOne could also imagine using treefmt because it caches results, but this will be slightly problematic when we change the formatter in the future and need to do the same multi-pass approach to avoid merge conflicts. There's going to be times when not the entire codebase is formatted in the same way, and treefmt would mess that up. |
That's kinda non-trivial to me, and you need to retrieve file list from Also I opened NixOS/nixfmt#211 for tracking this. |
Since it's the pre-commit hook, we only have to worry about the single commit that is being made, so I don't think it will be bad, but I'll see it when I try 😆. And we can use a |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
The good thing here is we can skip formatting if something weird really happens, because it doesn't make subsequent CI check fail. |
I’d like to recommend a Another advantage is that tools like If you need any help writing an efficient and correct Git hook for this, please ping me. |
a22dc49
to
a826121
Compare
174e6e4
to
00d4895
Compare
To make this PR easier to review, I split off the enforcement for new files into #326407. So this PR is only for the treewide format now! Also note:
|
ff4f5b6
to
0849c3c
Compare
b713881
to
a8ab3bb
Compare
a8ab3bb
to
0177070
Compare
Inactive means that there are no open PRs with activity in the last month that touch those files. A bunch later, we can do another pass to get the rest. Doing it this way makes ensures that we don't cause any conflicts for recently active PRs that would be ready to merge. Only once those PRs get updated, CI will kick in and require the files to be formatted. Furthermore, this makes sure that we can merge this PR without having to constantly rebase it! This can be verified using nix-build https://gist.github.com/infinisil/4b7a1a1e5db681d04446e73e39048aec/archive/5439e16eda66856aed747db3b45ed595835de7c6.tar.gz \ --argstr baseRev 5180ff0 result/bin/apply-formatting $NIXPKGS_PATH
0177070
to
0f1bf0d
Compare
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/enforcing-nix-formatting-in-nixpkgs/49506/1 |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/enforcing-nix-formatting-in-nixpkgs/49506/8 |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/formatting-team-meeting-2024-08-06/50222/1 |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/formatting-team-meeting-2024-09-03/51584/1 |
Note
Enforcement of formatting has been split into this separate PR: #326407
Description of changes
This is the next step towards accepted NixOS/rfcs#166 after #322512, see also #322520 and NixOS/nixfmt#153 🎉.
The @NixOS/nix-formatting will approve/merge this PR.
This PR formats all "inactive" files using nixfmt-rfc-style, where "inactive" means that there are no open PRs with activity in the last ~2 months that touch those files.
Doing it this way makes ensures that we don't cause any conflicts for recently active PRs that would be ready to merge. Only once those PRs get updated, CI will kick in and require the files to be formatted.
A bunch later, we can do another pass to get the rest.
Furthermore, this makes sure that we can merge this PR without having to constantly rebase it!
Verification
It's impossible to review this, but that's not necessary because you can review the code that generated it instead, which can be found here. At the same time this can be used to verify the contents of this PR by:
.git-blame-ignore-revs
(which changes because the date of the commit it creates changes)Things done
src = ./.
entries that unintentionally include their own.nix
files #301014)nix-instantiate --parse
doesn't changenix-instantiate --parse
from this:This work is sponsored by Antithesis ✨
Add a 👍 reaction to pull requests you find important.