treewide: switch back to alejandra formatting #1629

MattSturgeon · 2024-06-04T00:35:58Z

flake-modules: treefmt is formatter by default
flake-modules: switch fmt to alejandra
treewide: reformat with alejandra
.git-blame-ignore-revs: init

This PR switches back to using Alejandra to format the project, as discussed on matrix.

A git blame ignore-revs-file is included using a filename recognised by GitHub.
To use this locally, you should configure your git config as described in .git-blame-ignore-revs's header comments. Or see this section of the git manual.

In the future, it'd be nice to automatically add that config, e.g. via git-hooks and/or a devshell hook.

I kept the change in flake-modules and the actual treewide re-formatting in separate commits for ease of review, however they can be squashed if preferred.

Note to self: when squashing or rebasing, remember to update the sha in .git-blame-ignore-revs!!

Remove explicit `formatter = config.treefmt.build.wrapper`, because treefmt's `flakeFormatter` option (default `true`) handles that for us.

Sorry nixfmt-rfc-style; it's not you, it's me.

Initially added to ignore large-scale reformatting commits when using `git blame`. See [GitHub's Documentation][1] and the [`git blame` manual][2]. [1]: https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view [2]: https://www.git-scm.com/docs/git-blame#Documentation/git-blame.txt---ignore-revs-fileltfilegt

GaetanLepage · 2024-06-04T00:52:52Z

The changes look correct to me. The .git-blame-ignore-revs is a nice addition.

GaetanLepage · 2024-06-04T00:53:39Z

Of course, let's wait for @traxys's confirmation before merging such a change.

MattSturgeon

After scrolling through a decent portion of the diff and observing formatting differences, I'm less convinced this is a change worth making.

I do think we should discuss it further, but many of the improvements we were expecting from switching back to Alejandra don't seem to have happened.

There are a few cases where the formatting is better, but just as many where it is worse.

MattSturgeon · 2024-06-04T00:43:24Z

flake-modules/default.nix

+  perSystem = {
+    pkgs,
+    system,
+    ...
+  }: {


IMO nixfmt is better here:

Suggested change

perSystem = {

pkgs,

system,

...

}: {

perSystem = { pkgs, system, ... }: {

MattSturgeon · 2024-06-04T00:44:21Z

flake-modules/dev/devshell.nix

-{
-  imports = [ inputs.devshell.flakeModule ];
+{inputs, ...}: {
+  imports = [inputs.devshell.flakeModule];


Personally, I prefer whitespace in lists:

Suggested change

imports = [inputs.devshell.flakeModule];

imports = [ inputs.devshell.flakeModule ];

MattSturgeon · 2024-06-04T00:45:19Z

flake-modules/helpers.nix

-{
-  _module.args.getHelpers =
-    pkgs: _nixvimTests:
+{getHelpers, ...}: {


Again, I prefer the nixfmt padding:

Suggested change

{getHelpers, ...}: {

{ getHelpers, ... }: {

MattSturgeon · 2024-06-04T00:47:11Z

flake-modules/modules.nix

+        {lib, ...}:
+          with lib; {


I think I do prefer how alejandra indents this and keeps the opening bracket inline with the with statement.

nixfmt's indentation is less clear.

MattSturgeon · 2024-06-04T00:48:36Z

flake-modules/modules.nix

+                  #   description = null or "<DESCRIPTION>";
+                  #   url = null or "<URL>";
+                  # }
+                  merge = _: defs:


I like having function parameters inline with the attribute name, so this is an improvement.

MattSturgeon · 2024-06-04T00:58:38Z

lib/options.nix

+    mkStrLuaOr' = {
+      default,
+      description,
+      ...
+    } @ args:
+      mkNullOrStrLuaOr' (convertArgs args);


I think this is a downgrade.

MattSturgeon · 2024-06-04T00:59:58Z

lib/options.nix

+    mkEnum' = {
+      values,
+      default ? head values,
+      ...
+    } @ args:
+    # `values` is a list and `default` is one of the values (or null)
+      assert isList values;
+      assert default == null || elem default values;


The comment is indented poorly. IDK if alejandra allows manual intervention in this case, though.

MattSturgeon · 2024-06-04T01:01:52Z

lib/types.nix

-  logLevel = types.enum [
-    "off"
-    "error"
-    "warn"
-    "info"
-    "debug"
-    "trace"
-  ];
+    logLevel = types.enum [
+      "off"
+      "error"
+      "warn"
+      "info"
+      "debug"
+      "trace"
+    ];


This kind of multi-line list was one of @GaetanLepage's biggest complaints with nixfmt, however alejandra doesn't appear to be any different 😖

MattSturgeon · 2024-06-04T01:02:41Z

lib/utils.nix

@@ -11,69 +12,78 @@ with lib;
    "__empty" = null;
  };

-  /**
-    Turn all the keys of an attrs into raw lua.
+  /*


Alejandra doesn't support RFC145 docstrings!

There is a patch for this issue, though we probably want to avoid compiling a rust program each time we format our code

MattSturgeon · 2024-06-04T01:07:01Z

plugins/colorschemes/modus.nix

      helpers.defaultNullOpts.mkEnumFirstDefault
-        [
-          "default"
-          "tinted"
-          "deuteranopia"
-          "tritanopia"
-        ]
-        ''
-          Styles come in four variants:
-
-          - `default` is the plugin's main style designed to cover a broad range of needs.
-          - `tinted` tones down intensity and provides more color variety.
-          - `deuteranopia` is optimized for users with red-green color deficiency.
-          - `tritanopia` is optimized for users with blue-yellow color deficiency.
-        '';
+      [
+        "default"
+        "tinted"
+        "deuteranopia"
+        "tritanopia"
+      ]
+      ''
+        Styles come in four variants:
+
+        - `default` is the plugin's main style designed to cover a broad range of needs.
+        - `tinted` tones down intensity and provides more color variety.
+        - `deuteranopia` is optimized for users with red-green color deficiency.
+        - `tritanopia` is optimized for users with blue-yellow color deficiency.
+      '';


Another example of relatively short lists being formatted essentially the same.

GaetanLepage · 2024-06-04T01:14:42Z

This might need a deeper look indeed.
In terms of padding in lists/attrs, I don't really care.
I am not sure whereas the downgrades can be made better (while staying alejandra-conformant).
I know that it is possible to have (not too big) lists displayed on a single line.

Alejandra is probably less strict than nixfmt, so we might have some margin to adapt code here and there. To be confirmed though.

MattSturgeon · 2024-06-04T01:17:20Z

In terms of padding in lists/attrs, I don't really care.

For me it's not a big deal, also. But all else being equal, I find the nixfmt style, with whitespace, to be more readable.

I am not sure whereas the downgrades can be made better (while staying alejandra-conformant).
I know that it is possible to have (not too big) lists displayed on a single line.

We can use the Alejandra playground and Nixfmt online demo to experiment, I guess.

I'm not sure how I feel about attempting to make a few cases slightly better, if it still doesn't solve the overall issue. In general, I think we should avoid large sweeping changes (like this) unless they have a clear benefit.

Alejandra is probably less strict than nixfmt, so we might have some margin to adapt code here and there. To be confirmed though.

I'd be surprised if that is the case, but for sure worth investigating: Alejandra was originally marketed as a fully AST-based (and therefore deterministic) formatter, but that may have changed I guess.

To quote:

Not formatting everything is a limitation of an architecture based on rules
and not AST rebuilding: nixpkgs-fmt/src/rules.rs#L23 15

Some people see “not formatting” as a feature of a formatter,
I just simply can’t, that’s not the definition of a formatter.

A formatter formats, right?

MattSturgeon · 2024-06-04T01:38:36Z

Alejandra is probably less strict than nixfmt, so we might have some margin to adapt code here and there. To be confirmed though.

I'd be surprised if that is the case, but for sure worth investigating: Alejandra was originally marketed as a fully AST-based (and therefore deterministic) formatter, but that may have changed I guess.

Specifically on lists, I stand corrected. If your input code has the list on a single line, alejandra will avoid splitting it up.

So you could have two identical lists, formatted differently in alejandra.

As an example, this is valid code formatted by alejandra:

{
  foo = ["default" "tinted" "deuteranopia" "tritanopia"];
  bar = [
    "default"
    "tinted"
    "deuteranopia"
    "tritanopia"
  ];
}

Whether or not this non-determinism is a good or a bad thing, I'm undecided.

MattSturgeon · 2024-06-04T01:53:58Z

As for practical ways we could apply that formatting to 450 files, I'm at a loss.

For files with no changes since 62f32bf, we could simply checkout a copy of the file from the parent commit 62f32bf^ using a bash script. This should cover most files.

For other files, we'd either have to review every list on a case-by-case basis or write a small single-purpose custom formatting tool that decides based on list length and/or resulting line-length whether to format a list on one line or not.

If we do that, I think it should be done as part of the treewide reformat commit, so that it can be a single entry in the ignore-revs file.

That said, I'm not 100% convinced it is worth the effort.

This issue in particular is annoying, because it means we can't use RFC145 docstrings.
EDIT There is a patch available for that issue, though we probably want to avoid compiling a rust program each time we format our code...

traxys · 2024-06-04T07:53:48Z

That's one of the reasons I don't like alejandra much, it leaves a huge user choice in the mix, making it difficukt to enforce a style & having loads of PRs with only style reviews.
I'm not really favourable for it, but I am not against it if you decide otherwise

traxys · 2024-06-04T07:56:59Z

For me the biggest use of a formatter is to avoid any style comments in PRs (or at least try to have almost none). I feel like with nixfmt we have had much less of those, and I'd like that to continue, so I guess the options I'd like are either we stay on nixfmt, or we go back to alejandra but we mostly don't question how people format their code in accordance to alejandra

GaetanLepage · 2024-06-04T08:02:11Z

That's one of the reasons I don't like alejandra much, it leaves a huge user choice in the mix, making it difficukt to enforce a style & having loads of PRs with only style reviews.
I'm not really favourable for it, but I am not against it if you decide otherwise

I agree on the fact that a "strict" formatter is preferable. However, in this case, the question was worth considering as I find the nixfmt style very less readable than alejandra.

But not having to discuss style in PR reviews is a significant benefit of nixfmt... Not easy...

MattSturgeon · 2024-06-04T13:58:32Z

I've marked this as draft since discussion is ongoing and any decisions would likely involve changes to the PR.

I think we're all agreed that a strict formatter is preferable, however we sometimes find specific nixfmt choices less readable than manual formatting.

Frankly, to some degree this is inevitable; the stricter a formatter is the more edge case you'll run into where it doesn't work well for that specific scenario.

For short non-singleton lists, we may be able to patch nixfmt instead of resorting to an entirely different formatter... This would give us the "best of both worlds".

I doubt such a patch would be accepted upstream (maybe behind an option though?) since it was likely already discussed in the RFC, but we could maintain it as a nixvim patch or a standalone flake repo.

Are short non-singleton lists the only solid example where you prefer alejandra @GaetanLepage?

FYI the nixfmt standard describes this in the Expansion of expressions section. It does have good justifications, but I agree it is on-the-whole less readable in our codebase:

Long sequences of items should be liberally expanded, even if they would fit onto one line character-wise.

The motivation is to keep the information per line manageable. Usually "number of elements" is a better metric for that than "line length".

The cutoff is usually determined empirically based on common usage patterns.

GaetanLepage · 2024-06-05T06:49:42Z

I am willing to put my preferences aside considering the importance of strict formatting. This is indeed the most important feature of the formatter.
I am OK to let it go and focus on more important matters.
Thank you anyway for the in-depth exploration of the subject.

MattSturgeon · 2024-06-05T07:07:42Z

I'll close this as I think we're all agreed we don't want to do this. Fingers crossed NixOS/nixfmt#206 is accepted upstream or we are able to maintain our own patch.

MattSturgeon added 4 commits June 4, 2024 01:02

flake-modules: treefmt is formatter by default

e71511c

Remove explicit `formatter = config.treefmt.build.wrapper`, because treefmt's `flakeFormatter` option (default `true`) handles that for us.

flake-modules: switch fmt to alejandra

97fc5e7

Sorry nixfmt-rfc-style; it's not you, it's me.

treewide: reformat with alejandra

e9cab7f

MattSturgeon requested review from traxys and GaetanLepage June 4, 2024 00:36

GaetanLepage approved these changes Jun 4, 2024

View reviewed changes

MattSturgeon commented Jun 4, 2024

View reviewed changes

MattSturgeon marked this pull request as draft June 4, 2024 13:52

MattSturgeon mentioned this pull request Jun 4, 2024

flake-modules: minor tweaks from #1629 #1634

Merged

MattSturgeon closed this Jun 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

treewide: switch back to alejandra formatting #1629

treewide: switch back to alejandra formatting #1629

MattSturgeon commented Jun 4, 2024 •

edited

Loading

GaetanLepage commented Jun 4, 2024

GaetanLepage commented Jun 4, 2024

MattSturgeon left a comment

MattSturgeon Jun 4, 2024

MattSturgeon Jun 4, 2024

MattSturgeon Jun 4, 2024

MattSturgeon Jun 4, 2024

MattSturgeon Jun 4, 2024

MattSturgeon Jun 4, 2024

MattSturgeon Jun 4, 2024

MattSturgeon Jun 4, 2024

MattSturgeon Jun 4, 2024

MattSturgeon Jun 4, 2024

MattSturgeon Jun 4, 2024

GaetanLepage commented Jun 4, 2024

MattSturgeon commented Jun 4, 2024 •

edited

Loading

MattSturgeon commented Jun 4, 2024 •

edited

Loading

MattSturgeon commented Jun 4, 2024 •

edited

Loading

traxys commented Jun 4, 2024

traxys commented Jun 4, 2024

GaetanLepage commented Jun 4, 2024

MattSturgeon commented Jun 4, 2024 •

edited

Loading

GaetanLepage commented Jun 5, 2024

MattSturgeon commented Jun 5, 2024

	imports = [inputs.devshell.flakeModule];
	imports = [ inputs.devshell.flakeModule ];

treewide: switch back to alejandra formatting #1629

treewide: switch back to alejandra formatting #1629

Conversation

MattSturgeon commented Jun 4, 2024 • edited Loading

GaetanLepage commented Jun 4, 2024

GaetanLepage commented Jun 4, 2024

MattSturgeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GaetanLepage commented Jun 4, 2024

MattSturgeon commented Jun 4, 2024 • edited Loading

MattSturgeon commented Jun 4, 2024 • edited Loading

MattSturgeon commented Jun 4, 2024 • edited Loading

traxys commented Jun 4, 2024

traxys commented Jun 4, 2024

GaetanLepage commented Jun 4, 2024

MattSturgeon commented Jun 4, 2024 • edited Loading

GaetanLepage commented Jun 5, 2024

MattSturgeon commented Jun 5, 2024

MattSturgeon commented Jun 4, 2024 •

edited

Loading

MattSturgeon commented Jun 4, 2024 •

edited

Loading

MattSturgeon commented Jun 4, 2024 •

edited

Loading

MattSturgeon commented Jun 4, 2024 •

edited

Loading

MattSturgeon commented Jun 4, 2024 •

edited

Loading