Skip to content

nixfmt-tree: init #384857

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

Merged
dasJ merged 1 commit into from
Mar 4, 2025
+133 −0
Merged

nixfmt-tree: init #384857

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
133 changes: 133 additions & 0 deletions pkgs/by-name/ni/nixfmt-tree/package.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
{
lib,
runCommand,
buildEnv,
makeWrapper,
writers,
treefmt2,
nixfmt-rfc-style,
nixfmt-tree,

settings ? {
Copy link
Contributor

@MattSturgeon MattSturgeon Mar 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Way beyond scope for this PR, but would it make sense down the line to consider using evalModules to configure settings?

That way the default config could be partially overridden or extended using the module system.

Something like:

{
  lib,
  formats,
  # ...
  settings ? { },
}:
let
  defaults = {
    # ...
  };
  settingsFormat = formats.toml { };
  configuration = lib.evalModules {
    modules = [
      defaults
      settings
      { freeformType = settingsFormat.type; }
    ];
  };
  settingsTOML = settingsFormat.generate "treefmt.toml" configuration.config;
in

Copy link
Member Author

@infinisil infinisil Mar 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's where treefmt-nix comes in, because it does effectively exactly that (and more) :)

Copy link
Contributor

@MattSturgeon MattSturgeon Mar 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

treefmt-nix is great, but for simple cases maybe it's overkill.

I wonder if having modular settings here offers a nice middle ground of flexibility and simplicity, without needing another dependency.

Just food for thought, not necessarily feedback on this PR

# The default is warn, which would be too annoying for people who just care about Nix
on-unmatched = "info";
# Assumes the user is using Git, fails if it's not
tree-root-file = ".git/index";
Comment on lines +14 to +15
Copy link
Contributor

@MattSturgeon MattSturgeon Mar 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sadly this also assumes a non-bare git repo and not using multiple/separate worktrees.

I suspect it will be common for users to only want to modify tree-root-file. Modular settings would make that easier, but for now they'd need to do:

nixfmt-tree.override (old: {
  settings = old.settings // {
    tree-root-file = "some-other-file";
  };
})

Luckily it isn't a nested setting, so they won't need lib.recursiveUpdate.

I don't have a better default to suggest for now, I think this is more a fundamental issue with the way treefmt searches for the root of the tree.

Maybe down the line, treefmt would allow us to configure a more dynamic method of determining the root, such as defining a script that can be run to test a directory, instead of a plain filename that is tested for existence.

I'm not asking for changes: just raising the issue for the record.


formatter.nixfmt = {
command = "nixfmt";
includes = [ "*.nix" ];
};
},
runtimePackages ? [
nixfmt-rfc-style
],
}:
buildEnv {
name = "nixfmt-tree";

# Allows this derivation to be used as a shell providing both treefmt and nixfmt
paths = [ treefmt2 ] ++ runtimePackages;
pathsToLink = [ "/bin" ];

nativeBuildInputs = [
makeWrapper
];

postBuild = ''
wrapProgram $out/bin/treefmt \
--prefix PATH : $out/bin \
--add-flags "--config-file ${writers.writeTOML "treefmt.toml" settings}"
'';

meta = {
mainProgram = "treefmt";
description = "Official Nix formatter zero-setup starter using treefmt";
longDescription = ''
A zero-setup [treefmt](https://treefmt.com/) starter to get started using the [official Nix formatter](https://github.com/NixOS/nixfmt).

- For `nix fmt` to format all Nix files, add this to the `flake.nix` outputs:

```nix
formatter.''${system} = nixpkgs.legacyPackages.''${system}.nixfmt-tree;
```

- The same can be done more efficiently with the `treefmt` command,
which you can get in `nix-shell`/`nix develop` by extending `mkShell` using
Comment on lines +55 to +56
Copy link
Contributor

@MattSturgeon MattSturgeon Mar 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is using treefmt in a shell more efficient than using nix fmt?

Is this just because you don't need to use flakes and therefore don't need to copy the repo to the store before eval, or is there more to it?

Maybe a full explanation isn't needed in the long description, but mentioning efficiency without an explanation peaked my curiosity 😅

Copy link
Contributor

@MattSturgeon MattSturgeon Mar 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, again this should probably have a trailing : for consistency

Copy link
Member Author

@infinisil infinisil Mar 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this just because you don't need to use flakes and therefore don't need to copy the repo to the store before eval, or is there more to it?

Yes that, in addition to the Nix eval itself. On #380990, here are the rough times on my machine after changing a Nix file:

  • nix develop --command treefmt: 9.2s
  • nix fmt: 7.5s
  • nix-shell --run treefmt: 5.4s
  • treefmt (in nix develop/nix-shell): 0.2s


```nix
mkShell {
packages = [ pkgs.nixfmt-tree ];
}
```

You can then also use `treefmt` in a pre-commit/pre-push [Git hook](https://git-scm.com/docs/githooks)
and `nixfmt` with your editors format-on-save feature.

- To check formatting in CI, run the following in a checkout of your Git repository:
```
treefmt --ci
```

For more flexibility, you can customise this package using
Copy link
Contributor

@MattSturgeon MattSturgeon Mar 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: should probably also have a colon?

Suggested change
For more flexibility, you can customise this package using
For more flexibility, you can customise this package using:

Also very minor nit: some of your code blocks have a preceding blank line, while others do not. This should probably be consistent.

Copy link
Member Author

@infinisil infinisil Mar 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meta: I don't think it's even worth pointing out such small nits, it takes up way more time for the back-and-forth of writing the review and addressing it, than the value we get out of it 😅

Non-meta: There's a imo a good reason for this, it's because this makes sense grammatically (complete sentence before ":"):

For nix fmt to format all Nix files, add this to the flake.nix outputs: code.

While this doesn't:

For nix fmt to format all Nix files, add this to the flake.nix outputs code.

Whereas with the one here, this does make sense syntactically:

For more flexibility, you can customise this package using code.

While this doesn't (incomplete sentence before ":"):

For more flexibility, you can customise this package using: code.

Copy link
Contributor

@MattSturgeon MattSturgeon Mar 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meta: I don't think it's even worth pointing out such small nits, it takes up way more time for the back-and-forth of writing the review and addressing it, than the value we get out of it 😅

You're right, I wouldn't have commented if I wasn't already reviewing and wasn't clear about the (in)significance. I do get more nit-picky when it comes to user-facing prose...

Non-meta: There's a imo a good reason for this

If it was a deliberate decision please disregard my feedback. I wasn't sure, as you'd used it on some lines but not others.

Copy link
Member Author

@infinisil infinisil Mar 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do get more nit-picky when it comes to user-facing prose...

Imo that's best as a follow-up PR instead :)

If it was a deliberate decision please disregard my feedback. I wasn't sure, as you'd used it on some lines but not others.

All good ❤️

```nix
nixfmt-tree.override {
settings = { /* treefmt config */ };
runtimePackages = [ /* List any formatters here */ ];
}
```

Alternatively you can switch to the more fully-featured [treefmt-nix](https://github.com/numtide/treefmt-nix).
'';
# All the code is in this file, so same license as Nixpkgs
license = lib.licenses.mit;
maintainers = lib.teams.formatter.members;
platforms = lib.platforms.all;
};

passthru.tests.simple = runCommand "nixfmt-tree-test-simple" { } ''
export XDG_CACHE_HOME=$(mktemp -d)
cat > unformatted.nix <<EOF
let to = "be formatted"; in to
EOF

cat > formatted.nix <<EOF
let
to = "be formatted";
in
to
EOF

mkdir -p repo
(
cd repo
mkdir .git dir
touch .git/index
cp ../unformatted.nix a.nix
cp ../unformatted.nix dir/b.nix

${lib.getExe nixfmt-tree} dir
if [[ "$(<dir/b.nix)" != "$(<../formatted.nix)" ]]; then
echo "File dir/b.nix was not formatted properly after dir was requested to be formatted"
exit 1
elif [[ "$(<a.nix)" != "$(<../unformatted.nix)" ]]; then
echo "File a.nix was formatted when only dir was requested to be formatted"
exit 1
fi

(
cd dir
${lib.getExe nixfmt-tree}
)

if [[ "$(<a.nix)" != "$(<../formatted.nix)" ]]; then
echo "File a.nix was not formatted properly after running treefmt without arguments in dir"
exit 1
fi
)

echo "Success!"

touch $out
'';
}