let .. in balancing about alejandra

[zimbatm]

nixpkgs-fmt started the same way, where the rule would distinguish between top-level vs nested indents. In the end, I think it's easier to make that rule uniform. It simplifies the algorithm and also minimizes diff, even in the nested case.

[kamadorueda]

So just to confirm, implement, and go

We all agree that:

let
  foo = let
    foo = 123;
  in
    foo;
in
  foo

is better than:

let
  foo = let
    foo = 123;
  in
    foo;
in
foo

?

@zimbatm @0x4A6F @DavHau

Playground: link

[tomberek]

The first is more logically consistent, the second avoids diffs. I'm not convinced by the "less diffs when adding a top-level let", because it may even be of interest to be alerted to the fact that a top-level change has occurred and a new lexical scope was created. The other advantage of the first is that fewer special rules/edge-cases need to be implemented or explained to users. (and it's closer to what is in #71 😄 )

[kamadorueda]

• good: User is alerted of a new lexical scope
• good: Higher consistency
• bad: bigger initial nixpkgs diff
• bad: bigger diff when adding a let-in in a nested scope, whose target expression is big

I'm in with the change, let's wait for other people to add their comments

[tomberek]

Current version produces this at top level with minimal diffs.

let
  double = x: x + x;
in
{
  a = 1;
  b = 2;
  c = 3;
}

The difference is if this already indented/nested: https://github.com/kamadorueda/alejandra/blob/main/src/rules/let_in.rs#L97-L104

[kamadorueda]

I'm in with keeping the rules simple

There is also the case for RFC 101 where it seems to be preferable to minimize the initial diff on Nixpkgs, and that's mainly the reason why we difference a top-level from a nested let-in

I think the question is; do you prefer a balanced let in , or to minimize the diff when the code changes?

Other way to see the discussion is, do an average developer look more at code or at git diffs? From my perception, average people look more at code, and when dissecting git diffs with big indentation variations, there is git's --indent-heuristic, --minimal, --ignore-space-*, side-by-side diffs, and friends

I'm inclined to go for readability, which means enforcing rules, which avoids style discussions, and therefore a balanced let-in seems to be a good thing. But I'm also happy with both, external feedback was the decision maker here

[zimbatm]

A more realistic example would be:

{ stdenv, fetchurl }:
stdenv.mkDerivation {
  # 50 lines of declaration
}

Then I want to do something special with the src, maybe fetch different binaries depending on the arch:

{ stdenv, fetchurl }:
let
  # some sort of per-arch fetching
  src = {
    "x86_64-linux" = fetchurl ...;
  }.${stdenv.targetHost.system};
in
  stdenv.mkDerivation {
    # 50 lines get indented
  }

That kind of thing happens regularly in the codebase.

[piegamesde]

My 0.02€ on the topic:

• I have a weak preference for keeping the rules uniform and against special casing*
• Both sides of the let binding should be indented.
• For the in side however, the content should be indented. This keeps the diff low in many cases without sacrificing consistency

Examples:

let
  foo
in {
  bar = foo;
}

let
  pkgs = import <nixpkgs> {};
in with pkgs; [
  pkg1
  pkg2
  pkg3
]

let
  inherit (pkgs) lib;
in {
  foo,
  bar,
}: {
  inherit foo bar;
}

This approach leaves the question open where to draw the line as in how long a line may go. For example is in stdenv.mkDerivation rec { acceptable? If not, how do we indent it instead then?

Edit: Apparently, we already do in stdenv.mkDerivation rec { in some places in nixpkgs at the moment. Also, the same argument probably applies to function definitions as well.

*Edit 2: I just found https://kamadorueda.github.io/alejandra/?path=pkgs%2Ftest%2FmkOption%2Fkeep.nix as an example where the top-level rule goes horribly wrong. Therefore, I'm now stronger against top-level special casing than before.

[kamadorueda]

Solved in: #131