-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Automatically format with clang-tools #6721
Conversation
43c0701
to
8fd306b
Compare
I was dying for something like this when I tried to write some code for Nix a few days ago. Hope it happens! |
d3c648d
to
2c1d506
Compare
.completer = completePath | ||
}); | ||
addFlag( | ||
{.longName = "profile", |
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.
The change to the formatting of designated initializers causes a huge diff, but I don't see a way to keep the old style...
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.
We could use .git-blame-ignore-revs
as we do in nixpkgs
.
@@ -17,11 +17,14 @@ struct CmdLog : InstallableCommand | |||
std::string doc() override | |||
{ | |||
return | |||
#include "log.md" | |||
; | |||
#include "log.md" |
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.
This is ugly, it should follow the indentation.
7c763ab
to
7a584e9
Compare
I just squashed all the style update commits into one, rebased onto origin/master, and applieud a single reformat commit at the end. Also I changed .git-blame-ignore-revs to be a todo marker. |
BreakConstructorInitializers: BeforeComma | ||
EmptyLineAfterAccessModifier: Leave # change to always/never later? | ||
EmptyLineBeforeAccessModifier: Leave | ||
#PackConstructorInitializers: BinPack |
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.
This might help reduce the diff some, but the nixpkgs pinned in the flake has clang-format 13 and this option is introduced in 14.
Otherwise they don't survive reformatting, see the failure in NixOS#6721.
Include clang-format in dev shell only Co-authored-by: Eelco Dolstra <eelco.dolstra@determinate.systems>
…tyle Co-authored-by: Eelco Dostra <edolstra@gmail.com>
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.
The diff is obviously scary, but overall I think it's a great thing!
One thing that would be needed imho to make this really useful is a way to enforce this in the long run as it will quickly diverge from that style again. Maybe a CI job checking the format of everything?
Also, if the expectation is to enforce this, we should make it clear and easy for contributors by updating the hacking docs accordingly and having a nice make
target for it (I know there's already a script for it, but I don't think it's as discoverable and nice to use)
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/tweag-nix-dev-update-33/20048/1 |
TODO: add a CI check to verify formatting. |
Using the configuration file from NixOS#6721 for less conflicts
Using the configuration file from NixOS#6721 for less conflicts
Using the configuration file from NixOS#6721 for less conflicts
Using the configuration file from NixOS#6721 for less conflicts
Using the configuration file from NixOS#6721 for less conflicts
Using the configuration file from NixOS#6721 for less conflicts
Using the configuration file from NixOS#6721 for less conflicts
Using the configuration file from NixOS#6721 for less conflicts
Sort of a shitpost, but most times I read through Nix I wish it was consistently formatted.