Format the codebase with clang-format #12957
Conversation
github-actions
bot
added
documentation
new-cli
grahamc
force-pushed
the
check-format
branch
2 times, most recently
from
577f0b8 to
2bb5def
Compare
| .handler = {[&](std::string name, std::string expr) { autoArgs.insert_or_assign(name, AutoArg{AutoArgExpr{expr}}); }}, | ||
| .handler = {[&](std::string name, std::string expr) { | ||
| autoArgs.insert_or_assign(name, AutoArg{AutoArgExpr{expr}}); | ||
| }}, |
grahamc
force-pushed
the
check-format
branch
2 times, most recently
from
a4c85cf to
d3d1c7c
Compare
There was a problem hiding this comment.
Some issues to discuss in the Nix team meeting:
- What does this do to open PRs?
- Should this be backported to the stable branches? Generally we want to avoid giant diffs for point releases, but if we don't, it will make backporting much harder.
- Will this make git history largely useless?
In nixpkgs we have some tooling to fix existing pull requests after we reformatted everything with nixfmt. Maybe we can adapt the approach. I can also recommend to install https://mergiraf.org/ into your git locally as it is much more robust against whitespace changes than the git merge algorithm.
Probably. Otherwise our backport action becomes useless.
I believe .git-blame-ignore-revs should prevent this? |
grahamc
force-pushed
the
check-format
branch
5 times, most recently
from
d0aaa87 to
b242452
Compare
|
+1 on the things @Mic92 said. I want to try out those tools either way!
In addition to what @Mic92 said: Still, it might make sense to do a dedicated push trying to get the open PR count down first. Yes, that blocks this, but it is a good chore to do in its own right! So I don't mind the delay so much for that reason. |
|
The result of the last meeting that we all are for this change and we just need to figure out how we migrate old pull requests. |
|
FYI lix has a .clang-format file here, which you probably want to diff with and check if there's anything you want to change: https://git.lix.systems/lix-project/lix/src/branch/main/.clang-format We already went through the bikeshedding how to get the formatter to match existing formatting reasonably. |
|
This PR is using the existing .clang-format in Nix's repo fwiw |
|
Having our formatting be in sync does help cherry-pick things back and forth. |
grahamc
force-pushed
the
check-format
branch
3 times, most recently
from
2d73c29 to
d557902
Compare
* 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.
Motivation
Let's rip the bandaid off?
Note that PRs currently in flight should be able to be merged relatively easily by applying
clang-formatto 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.