Satisfaction survey from the new RFC 166 formatting?

Development
21

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.

22

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?

3 Likes
23

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?

24

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):

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.

2 Likes
25

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

AFAIR that principle went unchallenged in public.

1 Like
26

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!

1 Like
27

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.

2 Likes
28

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.

3 Likes
29

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

2 Likes
30

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

31

@nh2 Replied​​​​​ :slight_smile:

1 Like
32

Thanks, indeed that fixes the issue!

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

33

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

25 posts were split to a new topic: Complains about the adoption of the autoformatter RFC #166

35

I think arbitrary spaces are objectively conceptually inferior to 1 tab for each indentation level

36

Nix doesn’t support tabs at the start of indented strings, so that’s a nonstarter.

2 Likes
37

True.

Those would need to be merged first

38

Changing the semantics of the indentation stripping unfortunately is a blatant breaking change, so actually implementing this would require either a new syntax for tab-indented strings or (preferably) some language versioning infrastructure

2 Likes
39

Tabs are, in wide retrospect, an almost-perfect example of an attractive nuisance (I am personally a fan of the “smart tabs” code indentation style, but it is what it is)