add default nix formatter and its corresponding Github action #190
Conversation
drupol
force-pushed
the
ci/use-alejandra
branch
2 times, most recently
from
5ed2f6d to
8ec55db
Compare
drupol
force-pushed
the
ci/use-alejandra
branch
from
8ec55db to
49d51e9
Compare
|
@jtojnar WDYT about this? |
There was a problem hiding this comment.
I like this in principle and Alejandra feels closest to the ideal but there are still some cases where I feel like the formatting style hinders reading style where one is quickly scanning the code top-down (without moving much focus to the right).
I have wanted to play with it some more and argue for style changes where necessary upstream but have not found the time yet.
| nixpkgs, | ||
| utils, | ||
| }: | ||
| # For each supported platform, |
There was a problem hiding this comment.
Misaligned. Edit: Reported in kamadorueda/alejandra#372 (comment)
| }: | ||
| # For each supported platform, | ||
| utils.lib.eachDefaultSystem ( | ||
| system: let |
There was a problem hiding this comment.
Moving let to the same line as parameters is IMO inconsistent with indenting function bodies and makes visually scanning code harder (especially when the parameter list is long).
| pkgs: | ||
|
|
||
| # Overlay to be passed as packageOverrides to Nixpkgs’s generic PHP builder: | ||
|
|
||
| final: | ||
| prev: | ||
|
|
||
| let | ||
| final: prev: let |
There was a problem hiding this comment.
This is also weird, some arguments on single line and other not. Edit: reported in kamadorueda/alejandra#375
| callPackage = | ||
| cpFn: | ||
| cpArgs: | ||
|
|
||
| callPackage = cpFn: cpArgs: | ||
| prev.callPackage |
There was a problem hiding this comment.
Here again the arguments are moved away from the function body’s level, making it harder to notice it is a function body when scanning the code.
| prev.callPackage | ||
| cpFn |
There was a problem hiding this comment.
function
argument1
arg2
to me looks too much like a list of equal items so I have started indenting the arguments:
function
argument1
argument2
Will need to do more testing to see how it plays out in practice.
drupol
force-pushed
the
ci/use-alejandra
branch
from
49d51e9 to
9e67dfb
Compare
|
What we could do, is to use I saw many controversial things about IMHO, I like |
drupol
force-pushed
the
ci/use-alejandra
branch
from
9e67dfb to
7b6a610
Compare
|
Closing the PR until further notice. |
This PR:
alejandra--dry-runhasn't been implemented yet, seenix fmtshould have a--dry-runoption or similar NixOS/nix#6918)