Skip to content

let .. in balancing #60

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
zimbatm opened this issue Feb 10, 2022 · 9 comments
Closed

let .. in balancing #60

zimbatm opened this issue Feb 10, 2022 · 9 comments
Labels
component | style Modifications to the formatting rules status | ready We agree it is good to implement this type | feature request New feature or request

Comments

@zimbatm
Copy link

zimbatm commented Feb 10, 2022 •
edited
Loading

Consider the following starting example:

{
  a = 1;
  b = 2;
  #...
  z = 26;
}

Now let's say the code is evolving and I need to add a let .. in block on top:

let
  double = x: x+x;
in

In the current version, this pushes the whole attrset to the right, causing a lot of whitespace formatting diff.

I think the question is; do you prefer a balanced let <statement> in <body>, or to minimize the diff when the code changes? Both are valid choices IMO, it's just a matter of what you value most.
@zimbatm zimbatm changed the title let .. in reformatting let .. in balancing Feb 10, 2022
@kamadorueda kamadorueda added the component | style Modifications to the formatting rules label Feb 10, 2022
@tomberek
Copy link
Contributor

tomberek commented Feb 11, 2022 •
edited
Loading

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

@tomberek tomberek mentioned this issue Feb 11, 2022
Closed
@zimbatm
Copy link
Author

zimbatm commented Feb 11, 2022

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 kamadorueda added the status | needs discussing Needs discussion label Feb 12, 2022
@kamadorueda
Copy link
Owner

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

@kamadorueda kamadorueda added the type | question Further information is requested label Feb 14, 2022
@DavHau DavHau mentioned this issue Feb 14, 2022
Closed
@kamadorueda
Copy link
Owner

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
Copy link
Contributor

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
Copy link
Owner

kamadorueda commented Feb 17, 2022 •
edited
Loading

  • 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

@zimbatm
Copy link
Author

zimbatm commented Feb 17, 2022

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
Copy link
Contributor

piegamesde commented Feb 17, 2022 •
edited
Loading

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 kamadorueda added status | ready We agree it is good to implement this type | feature request New feature or request and removed type | question Further information is requested status | needs discussing Needs discussion labels Feb 17, 2022
@kamadorueda
Copy link
Owner

Solved in: #131

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
component | style Modifications to the formatting rules status | ready We agree it is good to implement this type | feature request New feature or request
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@zimbatm @tomberek @piegamesde @kamadorueda