-
-
Notifications
You must be signed in to change notification settings - Fork 15.3k
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.
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
nixfmt-tree: init #384857
nixfmt-tree: init #384857
Conversation
Depends on it being introduced with NixOS/nixpkgs#384857 first
cat > $out/libexec/treefmt.toml <<EOF | ||
# 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" | ||
|
||
[formatter.nixfmt] | ||
command = "nixfmt" | ||
includes = ["*.nix"] | ||
EOF |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like it'd be nicer to define this as a package attr - should be more readable and more overridable too.
E.g.
passthru.settings = {
# ...
};
settingsFile = writers.writeTOML "treefmt.toml" finalAttrs.passthru.settings;
Or maybe settings
makes more sense as a package argument, rather than a passthru? Or both?
c392fb8
to
8f03f02
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Non-critical formatting nits and somewhat out of scope discussion:
treefmt --ci | ||
``` | ||
|
||
For more flexibility, you can customise this package using |
There was a problem hiding this comment.
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?
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.
There was a problem hiding this comment.
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 theflake.nix
outputs: code.
While this doesn't:
For
nix fmt
to format all Nix files, add this to theflake.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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ❤️
- The same can be done more efficiently with the `treefmt` command, | ||
which you can get in `nix-shell`/`nix develop` by extending `mkShell` using |
There was a problem hiding this comment.
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 😅
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.2snix fmt
: 7.5snix-shell --run treefmt
: 5.4streefmt
(innix develop
/nix-shell
): 0.2s
nixfmt-rfc-style, | ||
nixfmt-tree, | ||
|
||
settings ? { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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) :)
There was a problem hiding this comment.
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
Motivation: NixOS/nixfmt#273
# Assumes the user is using Git, fails if it's not | ||
tree-root-file = ".git/index"; |
There was a problem hiding this comment.
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.
Would like somebody else from @NixOS/nix-formatting to also approve |
Brief notes from todays Nix formatting team meeting: |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/formatting-team-meeting-2025-03-04/61161/1 |
Makes it much more convenient to get a basic setup for the official Nix formatter. The long description has the story:
This mostly resolves NixOS/nixfmt#273.
Things done
flake.nix
This work is funded by Antithesis and Tweag ✨
Add a 👍 reaction to pull requests you find important.