Satisfaction survey from the new RFC 166 formatting?

[rhendric]

Merely allowing short lists to remain on a line, which is already supported by the RFC (just not the implementation), would go a long way towards fixing this particular explosion (and most other issues that I’ve seen in formatting my own code).

[Infinisil]

We just merged Maintain expanded attrs/lists/attr params/inherits by infinisil · Pull Request #224 · NixOS/nixfmt · GitHub, which also enables a better heuristic for short single-line lists

[rhendric]

I think that’s a different issue; that patch lets expanded short lists remain expanded, but it still expands collapsed short lists, which is one of the complaints raised here.

[Infinisil]

I should’ve clarified it more: Before Maintain expanded attrs/lists/attr params/inherits by infinisil · Pull Request #224 · NixOS/nixfmt · GitHub, if lists with 3 elements were allowed to be on a single line, this code:

buildInputs = [
  a
  b
  c
]

Would’ve been formatted as

buildInputs = [ a b c ];

Which is why it would’ve been a really bad choice. But now that expanded attribute sets are preserved, the above wouldn’t get reformatted, while also allowing the second style

[piegames]

@rnhmjoj

I really hope the plan is not to bulk-reformat the entirety of Nixpkgs

That indeed is exactly the plan. However while the RFC is done and fixes a lot of the format in place, I think there still is a lot of tweaking to do on the last mile before we go for the full deployment.

@rnhmjoj

My impression is that this formatter was made more with packages in

This is interesting to hear, because I’ve also heard criticism that the formatter was made for complex library code in mind and does handle packages that well. In the end we looked at all kinds of code to make tradeoffs, but I’d agree that the chosen format is at its weakest with module definitions.

[doronbehar]

@Infinisil

We just merged Maintain expanded attrs/lists/attr params/inherits by infinisil · Pull Request #224 · NixOS/nixfmt · GitHub , which also enables a better heuristic for short single-line lists

@Infinisil that’s great news, because this bothered me very much. I hope this update will get in as fast as possible .

In general, what bothered me the most and what made me write this post, was that I read the RFC, and I read and understood the comments I received during FCP, and all of a sudden I see what the implementation does and I don’t understand how could I have missed these rules…

Related to lists and the PR above, I still don’t understand what rules in the RFC make the following example a bad formatting (the example from my initial post):

@doronbehar

outputs = [
  "out"
] ++ lib.optionals condition [
  "bin"
  "dev"
];

Is it an implementation detail?

[doronbehar]

BTW to satisfy my initial complaint, another option would be to allow this:

{
  outputs =
    [
      "out"
    #]
    #++ lib.optionals stdenv.isDarwin [
    # "dev"
    # "man"
    ];
}

Then when that comment will be uncommented, it would not cause a big diff. What do you think @Infinisil ? This way maintainers would be able to start out with code formatted in such a way that future diffs will be clean.

[MattSturgeon]

Just to add to the discussion with an example of something that I think wasn’t formatted very well

{
  yazi_floating_window_winblend = defaultNullOpts.mkNullableWithRaw (types.ints.between 0
    100
  ) 0 "`0` for fully opaque and `100` for fully transparent. See :h winblend";
}

I like that an attempt was made to attach to the ( ) parenthesis (as per the RFC), however I think this specific example would be more readable formatted with one line per argument:

{
  yazi_floating_window_winblend =
    defaultNullOpts.mkNullableWithRaw
    (types.ints.between 0 100)
    0
    "`0` for fully opaque and `100` for fully transparent. See :h winblend";
}

That said, this is a specific example, I’m not sure I can come up with a general rule-based suggestion here.

Perhaps some heuristics when deciding whether to split up an expression within ( ) to prefer keeping the inner expression intact?

[MattSturgeon]

@doronbehar

Then when that comment will be uncommented, it would not cause a big diff. What do you think @Infinisil ? This way maintainers would be able to start out with code formatted in such a way that future diffs will be clean.

I think we could debate whether it makes sense to use comments to disable code in the first place; in a project tracked by version control it should be trivial to find removed code via file history, right?

---

Was there a ruling in the RFC to say that we cannot attach the opening [ when assigning a list concatenation expression rather than a single list literal?

If reducing diff sizes is the goal, then perhaps we should be formatting it as:

{
  outputs = [
    "out"
  ]
  ++ lib.optionals stdenv.isDarwin [
    "dev"
    "man"
  ];
}

Or even:

{
  outputs = [
    "out"
  ] ++ lib.optionals stdenv.isDarwin [
    "dev"
    "man"
  ];
}

Although, I’m sure this has been debated before, likely during the RFC.

If so, perhaps it’s worth re-visiting the debate now that the formatting has reached a wider audience?

[rhendric]

@MattSturgeon

I like that an attempt was made to attach to the ( ) parenthesis (as per the RFC), however I think this specific example would be more readable formatted with one line per argument:

This is a place where the RFC is under-specified, I think (implication is not that the RFC authors did a bad job, but rather that the formatting team has freedom to do what they think best in this case):

• As many arguments as possible must be fit onto the first line.
• If there is at most one multi-line argument that can be absorbed and all other arguments before/after fit onto a single line respectively, then that multi-line argument is absorbed.
• Otherwise, the first argument not fitting onto the first line will start a new line with indentation, and all subsequent arguments will start on their own line as well.
  • All arguments that are not on the same line as the function must be indented by one level.
• If the last argument is parenthesized, the parentheses should get absorbed while its body is put on a new line with indentation.
  • Exception: If the last argument is parenthesized and its body contains an absorbable term, an alternative and more compact layout may be used instead: The body gets compacted and its term absorbed.
    • In this case, the inner term may be force-expanded.
    • This results in less indentation for many common Nix idioms.

The ‘first argument not fitting onto the first line’ is (types.ints.between 0 100), and it must ‘start a new line with indentation’, and ‘all subsequent arguments will start on their own line as well’. So this example is incorrect unless ‘there is at most one multi-line argument that can be absorbed’. Does (types.ints.between 0 100) qualify as a multi-line argument? Does it depend on whether it was initially written on multiple lines? I don’t think the RFC ever said.

[rhendric]

@MattSturgeon

Was there a ruling in the RFC to say that we cannot attach the opening [ when assigning a list concatenation expression rather than a single list literal?

It’s not in the RFC proper, but a principle arrived at during internal discussions was

Only expressions that are formatted such that all but the first and last line are indented, could be absorbable

AFAIR that principle went unchallenged in public.

[MattSturgeon]

@rhendric

Only expressions that are formatted such that all but the first and last line are indented, could be absorbable

I can see the logic, that the ] ++ foo [ bit in the middle being un-indented could be confusing at a glance.

I don’t feel strongly either way, but I think it’s valuable to get feedback from users who missed that discussion and/or the RFC process entirely, so I’m glad this post was opened as an attempt to gather such feedback!

[Atemu]

@rhendric

Only expressions that are formatted such that all but the first and last line are indented, could be absorbable

AFAIR that principle went unchallenged in public.

After working with formatted code a bit and then reading to a non-formatted file, I got pretty confused at an expression that violated this rule for a good few seconds. This proves to me that it’s a pretty good rule.

[sudoforge]

absolutely agree with the need for short lists to remain on a single line. with a simple line like:

(someFunc [ a b c ] "potato" "bar")

it reformats it to:

(someFunc [
  a
  b
  c
] "potato" "bar")

which is just absolutely miserable. IMO, list items should not be broken out to a new line unless the entire line exceeds, say, 100 characters or something.

there are also things like this:

(exec [ mod ctrl ] "b" (builtins.toString [
  qutebrowser
  ''-B "${config.xdg.configHome}/qutebrowser/profiles/foo"''
  ''-C "${config.xdg.configHome}/qutebrowser/profiles/foo/config.py"''
]))

getting exploded into:

(exec
  [
    mod
    ctrl
  ]
  "b"
  (
    builtins.toString [
      qutebrowser
      ''-B "${config.xdg.configHome}/qutebrowser/profiles/foo"''
      ''-C "${config.xdg.configHome}/qutebrowser/profiles/foo/config.py"''
    ]
  )
)

which is far, far less readable and is just a giant waste of space. there’s absolutely no reason that each bracket and paren needs to be on its own line.

[piegames]

This has been bothering me for a while too, so I hacked together something small to address it: Don't expand lists within functions by piegamesde · Pull Request #233 · NixOS/nixfmt · GitHub

[nh2]

Is it expected that nixfmt makes package imports un-diffable / un-cherry-pickable, or am I using it wrong?

nh2

[Infinisil]

@nh2 Replied​​​​​

[nh2]

Thanks, indeed that fixes the issue!

I propose to slightly update the message to avoid such confusion:

nh2

[Sigmanificient]

I believe vm tests also gets unnecessary indentation, what use to be 2 simple lines

import ../make-test-python.nix ({ lib, ...}: {
  # ...
})

now becomes the following snippet, adding a extra unnecessary indent to

import ../make-test-python.nix (
  { lib, ... }:
  {
    # ...
  }
)