disko icon indicating copy to clipboard operation
disko copied to clipboard

Add --check option to formatter in flake

Open beviu opened this issue 1 year ago • 4 comments

I noticed that master is not formatted properly. Would it be interesting to add a CI check for formatting? This PR adds an option to nix fmt to check formatting without modifying files which could be added to buildbot.

beviu avatar Jul 22 '24 22:07 beviu

Very cool! Yes, checking the formatting in PRs automatically would be very nice, and this is good ground-work for that. Could you rebase the PR on current master?

iFreilicht avatar Sep 21 '24 00:09 iFreilicht

I think ideally we would replace the makeshift formatter script with one generated by a framework:

https://github.com/nix-community/nixos-anywhere/blob/34e1c624ec04d4aaddc22b07ff9de6186bd6ad0c/treefmt/flake-module.nix#L1-L17

or

https://github.com/Enzime/dotfiles-nix/blob/%F0%9F%94%A5/flake.nix#L359-L385

Enzime avatar Sep 21 '24 06:09 Enzime

Since this repository needs to be imported into many nixos configurations, we tried so far the keep the flake inputs at a minimum. However there is also a pattern to hide flake inputs that we only need for nix flake check: https://github.com/numtide/nixos-facter-modules/blob/6eb13404bbe40b9a5ef9984792b27933658571fa/flake.nix#L7

Mic92 avatar Sep 21 '24 08:09 Mic92

I think ideally we would replace the makeshift formatter script with one generated by a framework:

Sure, that's nicer. But this PR would be good enough in my book to get the ball rolling on this. A working solution now is better than a perfect one later.

Since this repository needs to be imported into many nixos configurations, we tried so far the keep the flake inputs at a minimum.

That's a good point to consider. I didn't know about this pattern, that would be very useful to incorporate Enzime's suggestion without any downsides.

iFreilicht avatar Sep 21 '24 12:09 iFreilicht

Alright I rebased and actually ran the formatter now. This already illustrates why we need PR check for this. I would be interested in adding that as well, but I'm not sure where to start. I'm not even sure how buildbot is triggered.

Are you alright with me merging this? Don't want to make that decision on my own.

iFreilicht avatar Sep 26 '24 13:09 iFreilicht