nix-community / nixpkgs-fmt

Nix code formatter for nixpkgs [maintainer=@zimbatm]
https://nix-community.github.io/nixpkgs-fmt/
Apache License 2.0
583 stars 32 forks source link

Wrong indentation on multi line if/then/else expression #262

Open DavHau opened 3 years ago

DavHau commented 3 years ago

Describe the bug the following source:

let
  nix = if !buildNix then null else
    buildRacketNix;
in
{ }

... is formatted to

let
  nix = if !buildNix then null else
  buildRacketNix;
in
{ }

Expected behavior A continuation of a expression in the next line should always be indented. System information

r-burns commented 3 years ago

Alternatively, maybe nixpkgs-fmt should move the trailing else to the next line before autoindenting. It appears that newline-before-else is much more common:

$ rg 'else$' | wc -l
1894

$ rg '^ *else' | wc -l
3870

Most of the cases of newline-after-else are actually because the else keyword is alone on the line.

$ rg '^ *else$' | wc -l
1193
zimbatm commented 3 years ago

Or maybe it should add \n after the =:

let
  nix =
    if !buildNix then null else
    buildRacketNix;
in
{ }
zimbatm commented 3 years ago

I agree that the current re-formatting is not ideal.

DavHau commented 3 years ago

Or maybe it should add \n after the =:

let
  nix =
    if !buildNix then null else
    buildRacketNix;
in
{ }

In my opinion yes. Whenever a = is followed by an expression spanning multiple lines, there should always be a \n after the =. Because otherwise those lines cannot be logically aligned correctly to each other. Especially with larger expressions this is very important for readability.

Also I totally think that if/else should almost always be formatted like this:

if bool_expression1 then
  indented_child_expression1
else if bool_expression2 then
  indented_child_expression2
else
  indented_child_expression3

Then again, there is the question of how much freedom the developer should be given. There are always some special cases where an alternative format can make sense.