TEST: reformat all nix files with nixpkgs-fmt

[domenkozar]

DO NOT MERGE

Testing #120832 with:

find . -name "*.nix" -exec $(nix-build -A nixpkgs-fmt)/bin/nixpkgs-fmt {} \;

[SuperSandro2000]

Thanks for starting this. Let us know if you need some help or some build power to test something.

[Synthetica9]

The commit message for the main commit here is crucial documentation that can't be changed afterwards; it is therefore critical to get this right. I'd propose something like:

treewide: format with nixpkgs-fmt

If you are seeing this in git blame, you have not set up .git-blame-ignore-revs properly. To do this, you need to run the following command once:

git config blame.ignoreRevsFile .git-blame-ignore-revs

This should work with all local tools.
For more information see [INFO URL].

The info URL is so we can give more information in the future.

[Mindavi]

I like the general idea of doing this.

However, there are some things I don't like about the current formatting:

• I don't think comments should be reformatted; they sometimes contain nix expressions, which are now flattened by the formatter (or it should be more intelligent, but I think not formatting is better)
• I saw something where multiple values were listed like below, and the formatting got weird

packageA,
# comment
packageB,
# comment

It was formatted to this:

packageA
, # comment
packageB
, # comment

The comments are really not great, the second thing is less of an issue, but I'd never manually format it like that.

(This was just with a brief look, haven't looked very long at this)

[grahamc]

I agree it should leave comments alone. A good example is these docs:

  /* Specialized `assertMsg` for checking if val is one of the elements
     of a list. Useful for checking enums.
    of a list. Useful for checking enums.
     Example:
       let sslLibrary = "libressl"
       in assertOneOf "sslLibrary" sslLibrary [ "openssl" "bearssl" ]
       => false
       stderr> trace: sslLibrary must be one of "openssl", "bearssl", but is: "libressl"
    Example:
    let sslLibrary = "libressl"
    in assertOneOf "sslLibrary" sslLibrary [ "openssl" "bearssl" ]
    => false
    stderr> trace: sslLibrary must be one of "openssl", "bearssl", but is: "libressl"
     Type:
       assertOneOf :: String -> ComparableVal -> List ComparableVal -> Bool
    Type:
    assertOneOf :: String -> ComparableVal -> List ComparableVal -> Bool
  */

the flattening here is unfortunate.

[domenkozar]

I like the general idea of doing this.

However, there are some things I don't like about the current formatting:

• I don't think comments should be reformatted; they sometimes contain nix expressions, which are now flattened by the formatter (or it should be more intelligent, but I think not formatting is better)
• I saw something where multiple values were listed like below, and the formatting got weird

packageA,

comment

packageB,

comment

It was formatted to this:

packageA
, # comment
packageB
, # comment

The comments are really not great, the second thing is less of an issue, but I'd never manually format it like that.

(This was just with a brief look, haven't looked very long at this)

That looks like nix-community/nixpkgs-fmt#243

[domenkozar]

I wanted to open this to test that whole nixpkgs/NixOS still evals. Unfortunately master moves quite fast and conflicts arise as soon as something is merged. Is there a way to run the whole eval suite locally?

[domenkozar]

Built nixpkgs-fmt from master and ran find . -name "*.nix" -exec /nix/store/askqcqa4mrblhpgyks60hxiph8c61haj-nixpkgs-fmt-1.2.0/bin/nixpkgs-fmt {} \; on fresh master.

[Mindavi]

This still doesn't look quite right:

https://github.com/NixOS/nixpkgs/pull/121490/files#diff-9a742acd363e7062932805c87186c92a025bef9d215fda52b8d46631691bbe9bR54-R62

But maybe that's intended, I'm not sure. It's not something I'd do by hand though.

But overall this looks a lot better to me!

[domenkozar]

Comma at the end of at the beginning is a debate I'd like to avoid here and what's important is that we can have either way as that is a lossless conversion.

Other things anyone can notice? Otherwise it seems like nixpkgs-fmt formats all files and eval checks pass. I've also tested that running it twice doesn't make any new modifications 🍿

[Ma27]

• Does anybody have a smart idea what tool to use for a review? 😅 git show in a less seems quite painful already ^^
• AFAICS also automagically created files are touched. Shouldn't we exclude these? (or fix the upstream tools)
• According to the labels this would cause a full stdenv rebuild (and from the changes I've seen in pkgs/stdenv this seems correct). Given that this is likely a candidate for staging, are we sure that this will be ready on time before the branchoff (which is on friday IIRC, cc @NixOS/release-engineers )

[zowoq]

AFAICS also automagically created files are touched. Shouldn't we exclude these? (or fix the upstream tools)

One auto-generated file looks like it is responsible for the majority of the diff in this PR.

 pkgs/development/haskell-modules/hackage-packages.nix
 1 file changed, 505371 insertions(+), 249022 deletions(-)

[asymmetric]

Comma at the end of at the beginning is a debate I'd like to avoid here and what's important is that we can have either way as that is a lossless conversion.

@domenkozar

This would look better IMO:

{
  foo
  # comments about bar
, bar
}

I think it would be valid too? Not sure how feasible or easy though.

[stale]

I marked this as stale due to inactivity. → More info