-
-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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
Document editorconfig and nixfmt, remove manual rules #330030
Conversation
, argN | ||
}: { } | ||
``` | ||
- New files must be formatted by entering the `nix-shell` from the repository root and running `nixfmt`. |
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.
CI also checks for files that already exists
- New files must be formatted by entering the `nix-shell` from the repository root and running `nixfmt`. | |
- New and edited files must be formatted by entering the `nix-shell` from the repository root and running `nixfmt`. |
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.
The check is for new and already-formatted files, not all edited files.
But it's probably more confusing to say "already-formatted" as you have to explain how to check that in turn... and that ratchet will eventually tighten anyway
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.
yeah, "Loop through all Nix files touched by the PR"
,
maybe New and Edited
files?
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.
But again, it's not all edited files, nor do we necessarily want people formatting, say,all-packages.nix
or other unformatted files with a high chance of merge conflicts
I think the sentence is fine as-is; if there's an issue with a file, CI will flag it and it's the same process to rectify it.
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.
Any suggestion then?
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.
As @eclairevoyant also said, it's not all edited ones, see #326407 for more context :)
I'd say it's okay to just say "New files" here, for now leaving it undefined how existing files should be formatted. Once we enforce it for all files (probably in like 2 months or so), we can change this to "All files".
Edit: Oh we're fast with replies, didn't see your replies in advance!
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.
Okay, just wanted to be sure that this was mentioned, just in case
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.
Just so we are clear, nixfmt
in the nix-shell
is from the nixfmt-rfc-style
package, right?
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.
Thank you for documenting the change so quickly!
Thanks for the quick reviews, I see no reason to wait with merging this, let's go! 🚀 |
Description of changes
This documents editorconfig and nixfmt in CONTRIBUTING.md. Ping @NixOS/nix-formatting @eclairevoyant
Add a 👍 reaction to pull requests you find important.