Skip to content

nix fmt: call nixfmt-rfc-style #11252

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

Jump to bottom
Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

nix fmt: call nixfmt-rfc-style #11252

wants to merge 1 commit into from

Conversation

tomberek
Copy link
Contributor

@tomberek tomberek commented Aug 4, 2024

Motivation

Make nix fmt usable by default.

  • Evaluating for .#formatters.SYSTEM or inputs.nixpkgs is too slow for the moment, so disabling that override for now.
  • Using the same mechanism as nix develop to obtain an ambient nixpkgs
  • defaulting to nixfmt-rfc-style
  • could read+evaluate ci/pinned-nixpkgs.json, this can cost another download+copy to store, but will be more stable over time in case someone has flake:nixpkgs set to a moving branch

Context

#6087

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

@github-actions github-actions bot added documentation new-cli Relating to the "nix" command labels Aug 4, 2024
@infinisil infinisil mentioned this pull request Aug 6, 2024
Closed
15 tasks
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/formatting-team-meeting-2024-08-06/50222/2

roberth
roberth reviewed Aug 6, 2024
View reviewed changes
src/nix/fmt.cc
if (nixfmt == "nixfmt") {
try {
auto res = nixpkgs.fetchTree(store);
auto s = store->printStorePath(res.first) + "/ci/pinned-nixpkgs.json";
Copy link
Member

Choose a reason for hiding this comment

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

This elevates a very recent Nixpkgs implementation detail to a new Nix interface that is not consistent with anything that came before in Nix. It would be more appropriate to do this in a Nixpkgs CLI.

What if we could just load the shell instead, or just stick to the plan and finish lazy trees?

Copy link
Contributor Author

@tomberek tomberek Aug 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Agreed, this is quite fragile. What about having a version pinned in Nix itself? A reference to nixpkgs or nixfmt repos. Benefit is thst it is simple and predictable. Downside is that you don't get an automatic upgrade.

Nixpkgs-specific tooling can do additional work to match the Nixpkgs to the fmt.

@tomberek
Copy link
Contributor Author

tomberek commented Aug 8, 2024 •
edited
Loading

User research notes (done on Discord):

  • nix fmt FILE1 FILE2 would only format those files.
  • nix fmt: would expect this to recurse through directory and format all ".nix"
    • would want a dry-run feature
    • would expect the version of nixfmt to be tied to Nix CLI
  • to change to a different formatter:
    • would expect this to be done by... a flag like "--version", but that is occupied, "--use-version"?
  • how to persist the changed version?
    • .cache

@roberth
Copy link
Member

roberth commented Aug 8, 2024

That all seems rather ad hoc. What if we just execed nixfmt from PATH? That way you can have a default one in your NixOS config, matching your NixOS pins, or you'd get the shared correct version from the nix shell you've loaded.
Otherwise we might end up recreating a bad implementation of treefmt and a new locking mechanism that we don't really need.

@tomberek
Copy link
Contributor Author

tomberek commented Aug 8, 2024 •
edited
Loading

I'm thinking of the following order.

  1. nixfmt in PATH (allows people who care to keep moving)
  2. pinned version associated with each CLI release, stable and easy to understand
  3. when feasible, pinned via repo (not today)

@roberth
Copy link
Member

roberth commented Aug 10, 2024

Isn't 3 mostly covered by the formatter flake attribute?

@infinisil
Copy link
Member

(ping @NixOS/nix-formatting)

@infinisil infinisil mentioned this pull request Jan 7, 2025
Closed
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/formatting-team-meeting-2025-04-01/62524/1

@roberth
Copy link
Member

roberth commented Apr 1, 2025

Meanwhile, this was done in #12349

nix/maintainers/flake-module.nix

Lines 40 to 41 in 9ed5482

nixfmt-rfc-style = {
enable = true;

We have a few exclusions for parser tests, but all the other nix files are formatted and enforced by CI and opt-in commit hook and a script.

@roberth roberth closed this Apr 1, 2025
@github-project-automation github-project-automation bot moved this from Todo to Done in Nix formatting Apr 1, 2025
@roberth
Copy link
Member

roberth commented Apr 1, 2025

Nvm, this is about nix fmt

@roberth roberth reopened this Apr 1, 2025
@roberth roberth changed the title WIP: feat(fmt): use nixfmt-rfc-style nix fmt: use nixfmt-rfc-style Apr 1, 2025
@roberth roberth changed the title nix fmt: use nixfmt-rfc-style nix fmt: call nixfmt-rfc-style Apr 1, 2025
@roberth roberth removed the status in Nix formatting Apr 1, 2025
@jfly jfly moved this to Approved in Nix formatting Jul 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roberth roberth roberth left review comments

@edolstra edolstra Awaiting requested review from edolstra edolstra will be requested when the pull request is marked ready for review edolstra is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
documentation new-cli Relating to the "nix" command
Projects
Nix formatting
Status: Approved
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tomberek @nixos-discourse @roberth @infinisil