Add formatter and CI formatting check

[eureka-cpu]

Closes #56

Reformats the project using the RFC style formatter. Specifies the formatter package in the flake. Adds a check to CI on pull request that fails if nix files are not properly formatted.

[kuruczgy]

Cool! Some quick ideas, I am on my phone right now so can't do a proper review yet:

• add a .git-blame-ignore-revs file (obviously that has to be done in a second commit)
• maybe add instructions about formatting to the README? I don't think we have a "contributing" section yet but maybe we should...

[kuruczgy]

Fuck whyyy. When I nix fmt I get this warning:

Passing directories or non-Nix files (such as ".") is deprecated and will be unsupported soon, please use https://treefmt.com/ instead, e.g. via https://github.com/numtide/treefmt-nix

See: NixOS/nixfmt#151

So we either have to depend on https://github.com/numtide/treefmt-nix (not a fan of adding a flake dependency for such a trivial thing), or roll our own with something like find -name '*.nix' or git ls-files -- '*.nix'.

[eureka-cpu]

Fuck whyyy. When I nix fmt I get this warning:

Passing directories or non-Nix files (such as ".") is deprecated and will be unsupported soon, please use https://treefmt.com/ instead, e.g. via https://github.com/numtide/treefmt-nix

See: NixOS/nixfmt#151

So we either have to depend on https://github.com/numtide/treefmt-nix (not a fan of adding a flake dependency for such a trivial thing), or roll our own with something like find -name '*.nix' or git ls-files -- '*.nix'.

Yeah I saw that warning. It's coming from the rfc formatter (https://github.com/NixOS/nixfmt/blob/a4639036723e510d8331124c80d9ca14dd7aba02/main/Main.hs#L121). Imo this is a really dumb warning, since nix fmt is just supposed to use the formatter provided by the flake. Since the rfc formatter doesn't support anything except for nix files, it doesn't really make sense that it would try to format other files even if it is the package the flake is using as the formatter. Just my opinion though.

[eureka-cpu]

To be honest, you would probably be better off using a more stable formatter like alejandra or nixpkgs-fmt. The readme for the rfc formatter indicates it is still extremely unstable software.

[kuruczgy]

What about something like this for formatting/checking all files? This should handle both tracked and untracked files, and also respect .gitignore:

git ls-files --cached --others --exclude-standard -z -- '*.nix' | xargs -0 nixfmt

Regarding nixfmt, I fear that it's in the "technically experimental but all other reasonable options are already deprecated" territory, same as flakes. They are already reformatting huge swaths of nixpkgs with it, look at this PR from 5 days ago (+706,935 -433,885): NixOS/nixpkgs#322537
If it's good enough for nixpkgs, I don't see why it would not be good enough for us.

[eureka-cpu]

What about something like this for formatting/checking all files? This should handle both tracked and untracked files, and also respect .gitignore:

git ls-files --cached --others --exclude-standard -z -- '*.nix' | xargs -0 nixfmt

Regarding nixfmt, I fear that it's in the "technically experimental but all other reasonable options are already deprecated" territory, same as flakes. They are already reformatting huge swaths of nixpkgs with it, look at this PR from 5 days ago (+706,935 -433,885): NixOS/nixpkgs#322537 If it's good enough for nixpkgs, I don't see why it would not be good enough for us.

I suppose something like that would work just fine. For now it seems this is only a warning, so maybe it's not worth worrying over?

[kuruczgy]

Pushed a change introducing git ls-files.

TODOs:

• Describe usage in README
• Test CI (do I have to enable it in the repo or something?)
• Add .git-blame-ignore-revs file

[kuruczgy]

Also .git-blame-ignore-revs might be problematic, because it will also ignore relevant changes from the reformat commit like the ci.yml or the formatter definition... maybe we should split it into two commits, the first just adds the formatter and CI but does not change anything else, and then the next one is just a pure reformat commit.

[infinisil]

Hey, (a) nixfmt maintainer here! We very intentionally deprecated that recursive mode, because the details get tricky, but it's all behavior generic for all formatters, making it perfect for treefmt. This includes:

• Specifying the files to format, in particular also avoiding "files" like .git/logs/refs/remotes/foo/bar.nix.
• Formatting files efficiently in parallel.
• Avoiding formatting of files that don't need it (using a cache).

If you want to see treefmt-nix in practice, you can check out the config in nixfmt itself. The definition of nixfmtEval is here, then you can build treefmtEval.config.build.check source to get something that fails in CI with a good error and add treefmtEval.config.build.wrapper to the development shell to get a locally runnable treefmt command.

Anyways, totally up to you how you want to handle it, but feel free to let us know of any problems or suggestions for improvement in the nixfmt issue tracker :)

Oh and you can consider nixfmt stable, we plan to make the 1.0 release very soon, the formatting should only change in minor ways going forward.

[kuruczgy]

@infinisil wow, definitely did not expect you to comment here, thank you for chiming in :) It's good to hear that nixfmt will be "officially" stabilized soon.

Yeah, fine, I did check out treefmt and treefmt-nix since you are recommending it. I read through some of the code in both, my only nitpick is that the flake example in the treefmt-nix readme is broken. (treefmtEval is an attrset of systems but the code is trying to refer to its config attribute.) Other than this both projects seem reasonable, I did not find anything objectionable.

(xargs does apparently have a parallel option, so my one-liner would only be missing caching from the features you listed if I used that.) But I do get the value proposition of treefmt and treefmt-nix now, and in fact they seem like something I might want to use in future projects with more complex formatting requirements using multiple formatters.

And at that point I might as well start using it here already. Getting some experience with it and learning any of its quirks on a simpler project is probably more valuable than avoiding the extra dependency in favor of the one-liner that technically would also work in this particular scenario.

I updated the PR to use treefmt-nix.

[kuruczgy]

Approved from my side. @eureka-cpu I did quite a few changes to your original PR, so you should also take a look to see if you agree with all of my changes.

[eureka-cpu]

LGTM thanks for the refactors @kuruczgy