-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Rip off the bandaid: Format the codebase with clang-format #13108
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
Conversation
@grahamc so I think the consense was that we can merge this if we have a strategy how to deal with existing pull requests. Does the script that works in nixpkgs for nixfmt also works with clang-format to rebate pull requests? I didn't got a chance yet the try it out but if this is done, I'll hit merge. |
@Mic92 cool! Can you link me to the script? |
https://github.com/NixOS/nixpkgs/pull/363759/files |
oh, fancy! =) I'll take a look! |
Did you had a chance to look at it? |
8eca3a2
to
fb60ae4
Compare
Ok! I gave this a go, and it worked well on some PRs when run like this:
note that I had to add gnused to the dev shell for it to work on a mac. However, I did try it on a more complicated PR and I don't understand what it is trying to tell me:
so I fixed the merge conflict, and wasn't sure if I should run the script immediately after fixing the edits, after running |
@grahamc you did replace nixfmt with clang-format in run.sh, right? |
Yeah, and actually that script doesn't call nixfmt, it uses the command specified in the .git-blame-ignore-revs. On that PR, I think it actually makes more sense to just do a big format on the tips and merge from there. So in other words, I think it is actually working perfectly. |
So all we need to do after merging this is: |
I think so! Want to test it on a few? |
For some reason it didn't work the first time I tried it, but the second time? |
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.
As good as it'll be. Let's get this done!
Besides the usual benefits, I'm particularly looking forward to enable format on save in my editor.
YESSS |
* It is tough to contribute to a project that doesn't use a formatter, * It is extra hard to contribute to a project which has configured the formatter, but ignores it for some files * Code formatting makes it harder to hide obscure / weird bugs by accident or on purpose, Let's rip the bandaid off? Note that PRs currently in flight should be able to be merged relatively easily by applying `clang-format` to their tip prior to merge.
@xokdvium we should also do the backport of all of this if we don't want backports to be a pain. |
Yeah, I've triggered the backports and will do the formatting/ignore-revs manually in a bit. |
…3108 Rip off the bandaid: Format the codebase with clang-format (backport #13108)
…3108 Rip off the bandaid: Format the codebase with clang-format (backport #13108)
…3108 Rip off the bandaid: Format the codebase with clang-format (backport #13108)
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/2025-07-23-nix-team-meeting-minutes-236/67114/1 |
I accidentally deleted the branch from the original PR. A brief summary:
Directional +'s from @roberth and @Ericson2314. Some discussion about updating the formatting one way or another around bracing, which was conceded by @roberth in interest of moving forward. This was brought up again re Lix's format changes, but no resolution came from that.
Eelco had concerns and @Mic92 provided what seemed like positive support and answers. Those concerns (and answers) were what about existing PRs (nixpkgs has tools to help), should it backport (probably), and if the git history becomes useless (
.git-blame-ignore-revs
, already done in this PR solves it.)Anything I can do to help move this forward?
original PR body and the regeneration script follows
Motivation
Let's rip the bandaid off?
Note that PRs currently in flight should be able to be merged relatively easily by applying
clang-format
to their tip prior to merge.I would also expect it to be not too hard to apply patch backports if the backport targets are similarly formatted with clang-format.
Context
Implementation strategy requires a bit of effort on each branch in active maintenance:
Once that is done, backports should be easy to apply.
Maintainers can focus on only the change in the first commit, and optionally completely regenerate the second commit to ensure it wasn't tampered with.
This has been done somewhat: new files must be formatted. However, that makes it challenging to contribute with editor integration for code formatting.
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.
Regeneration script below...