Satisfaction survey from the new RFC 166 formatting?

[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, ... }:
  {
    # ...
  }
)