Skip to content
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

Closed
wants to merge 5 commits into from

Conversation

grahamc
Copy link
Member

@grahamc grahamc commented Jun 24, 2022

Sort of a shitpost, but most times I read through Nix I wish it was consistently formatted.

@lovesegfault
Copy link
Member

I was dying for something like this when I tried to write some code for Nix a few days ago. Hope it happens!

@edolstra edolstra force-pushed the format branch 2 times, most recently from d3c648d to 2c1d506 Compare June 25, 2022 19:02
.completer = completePath
});
addFlag(
{.longName = "profile",
Copy link
Member

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...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -17,11 +17,14 @@ struct CmdLog : InstallableCommand
std::string doc() override
{
return
#include "log.md"
;
#include "log.md"
Copy link
Member

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.

@grahamc grahamc force-pushed the format branch 2 times, most recently from 7c763ab to 7a584e9 Compare June 27, 2022 13:38
@grahamc
Copy link
Member Author

grahamc commented Jun 27, 2022

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
Copy link
Member Author

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.

edolstra added a commit to edolstra/nix that referenced this pull request Jun 29, 2022
Otherwise they don't survive reformatting, see the failure in
NixOS#6721.
grahamc and others added 5 commits June 29, 2022 12:25
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>
Copy link
Member

@thufschmitt thufschmitt left a 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)

@nixos-discourse
Copy link

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

@edolstra
Copy link
Member

edolstra commented Feb 3, 2023

TODO: add a CI check to verify formatting.

infinisil added a commit to tweag/nix that referenced this pull request Feb 17, 2023
Using the configuration file from NixOS#6721 for less conflicts
infinisil added a commit to tweag/nix that referenced this pull request Feb 24, 2023
Using the configuration file from NixOS#6721 for less conflicts
infinisil added a commit to tweag/nix that referenced this pull request Feb 24, 2023
Using the configuration file from NixOS#6721 for less conflicts
infinisil added a commit to tweag/nix that referenced this pull request Feb 24, 2023
Using the configuration file from NixOS#6721 for less conflicts
@roberth roberth mentioned this pull request Feb 26, 2023
@fricklerhandwerk fricklerhandwerk added the contributor-experience Developer experience for Nix contributors label Mar 3, 2023
yorickvP pushed a commit to tweag/nix that referenced this pull request Apr 14, 2023
Using the configuration file from NixOS#6721 for less conflicts
yorickvP pushed a commit to tweag/nix that referenced this pull request Apr 14, 2023
Using the configuration file from NixOS#6721 for less conflicts
yorickvP pushed a commit to tweag/nix that referenced this pull request Apr 14, 2023
Using the configuration file from NixOS#6721 for less conflicts
yorickvP pushed a commit to tweag/nix that referenced this pull request Apr 19, 2023
Using the configuration file from NixOS#6721 for less conflicts
@grahamc grahamc closed this Oct 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contributor-experience Developer experience for Nix contributors
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants