Skip to content
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

Add nixfmt #772

Merged
merged 1 commit into from
Jul 11, 2024
Merged

Add nixfmt #772

merged 1 commit into from
Jul 11, 2024

Conversation

chris-martin
Copy link
Contributor

No description provided.

@CLAassistant
Copy link

CLAassistant commented Jul 1, 2024

CLA assistant check
All committers have signed the CLA.

@pbrisbin
Copy link
Member

pbrisbin commented Jul 1, 2024

Is this a competitor to https:/restyled-io/restylers/blob/main/nixpkgs-fmt/info.yaml ? Does it make sense to enable both or do we need to pick one to be our default?

@chris-martin
Copy link
Contributor Author

chris-martin commented Jul 1, 2024

nixfmt is on track to be the official standard Nix formatter, so it should probably be the default.

https:/NixOS/nixfmt

https:/NixOS/rfcs/blob/master/rfcs/0166-nix-formatting.md

It is somewhat unclear to me whether the RFC version is going to continue to be called "nixfmt" going forward, but nixfmt is being used as the basis for the standard. I think maybe what's happening is that the nixfmt tool is going to be synonymous with the standard, but I'm not sure.

@chris-martin
Copy link
Contributor Author

chris-martin commented Jul 1, 2024

Doing some more reading - Apparently Nix's idea of how formatting works is that the flake is responsible for providing the formatter it wants to use. You just run nix fmt, and it runs it. I'm not sure how exactly this fits into the Restyled paradigm. In some sense maybe it makes things easier because Restyled shouldn't need to be aware of what formatter it's using at all. We could just define a restyler called "nix flake" or something that runs whatever executable the flake declares as its formatter. However, it means that Restyled would have to understand that it needs to find flake.nix files and run nix fmt within each flake directory.

@pbrisbin
Copy link
Member

pbrisbin commented Jul 2, 2024

You just run nix fmt, and it runs it. ... We could just define a restyler called "nix flake" or something

Because the command is nix fmt, I think I'd call this nix-fmt. This follows the convention established by dhall-format, dart-format, terraform-fmt and others, where a tool with a "format" subcommand gets hyphenated.

I'm not sure how exactly this fits into the Restyled paradigm. In some sense maybe it makes things easier because Restyled shouldn't need to be aware of what formatter it's using at all. [It] runs whatever executable the flake declares as its formatter

That sounds right.

However, it means that Restyled would have to understand that it needs to find flake.nix files and run nix fmt within each flake directory

I'm not sure I follow "within each flake directory". There wouldn't just be one ./flake.nix and you run nix fmt in the current directory (in the common case at least)?

All Restyled does is mount the current directory (the checked out project repository) as /code, go into that directory, and run the defined command. This can be a simple command (nix fmt) or a script that does other stuff if necessary (though of course we try to avoid that complexity when possible). The one major caveat is that restylers have no network access, so they cannot lazy load plugins (for example). Everything that's needed must be pre-installed or exist within the project sources.

@chris-martin
Copy link
Contributor Author

There wouldn't just be one ./flake.nix and you run nix fmt in the current directory (in the common case at least)?

Probably a lot of repos have a flake.nix at the top level. Our megarepo has five. freckle/flakes only has one at the moment but we put it in a subdirectory to leave open the possibility of more.

The one major caveat is that restylers have no network access

Oh, that's a dealbreaker on nix fmt then.

(The nix fmt discussion has been a distraction, I think unrelated to the nixfmt PR.)

@pbrisbin
Copy link
Member

pbrisbin commented Jul 2, 2024

Oh, that's a dealbreaker on nix fmt then.

We should open a new Issue and discuss this further. Maybe there are ways around, or we can at least document it for when folks ask why it doesn't exist.

I think unrelated to the nixfmt PR.)

So would you like to proceed with this PR with enabled: true and mark nixpkgs-fmt enabled: false? I think one of them has to be blessed; I don't know or care which.

@chris-martin
Copy link
Contributor Author

with enabled: true and mark nixpkgs-fmt enabled: false?

Ah, didn't know how to do that. Will enable nixfmt.

@pbrisbin pbrisbin enabled auto-merge (rebase) July 4, 2024 13:10
auto-merge was automatically disabled July 5, 2024 16:54

Head branch was pushed to by a user without write access

@chris-martin chris-martin force-pushed the nixfmt branch 2 times, most recently from deef93d to 36b0b15 Compare July 8, 2024 21:46
@pbrisbin
Copy link
Member

pbrisbin commented Jul 9, 2024

Would you like some help running the test suite locally? I'm sensing a long feedback loop going on here.

@chris-martin
Copy link
Contributor Author

Sigh, alright, I installed the SDK thing. Test passes locally.

@pbrisbin pbrisbin enabled auto-merge (rebase) July 11, 2024 13:08
@pbrisbin pbrisbin merged commit 3ede8d7 into restyled-io:main Jul 11, 2024
57 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants