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

Document editorconfig and nixfmt, remove manual rules #330030

Merged
merged 2 commits into from
Jul 25, 2024

Conversation

infinisil
Copy link
Member

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.

@infinisil infinisil mentioned this pull request Jul 25, 2024
13 tasks
, argN
}: { }
```
- New files must be formatted by entering the `nix-shell` from the repository root and running `nixfmt`.
Copy link
Member

@Sigmanificient Sigmanificient Jul 25, 2024

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

Suggested change
- 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`.

Copy link
Contributor

@eclairevoyant eclairevoyant Jul 25, 2024

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

Copy link
Member

@Sigmanificient Sigmanificient Jul 25, 2024

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?

Copy link
Contributor

@eclairevoyant eclairevoyant Jul 25, 2024

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.

Copy link
Member

Choose a reason for hiding this comment

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

Any suggestion then?

Copy link
Member Author

@infinisil infinisil Jul 25, 2024

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!

Copy link
Member

@Sigmanificient Sigmanificient Jul 25, 2024

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

Copy link
Contributor

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?

Copy link
Contributor

@eclairevoyant eclairevoyant left a 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!

@infinisil
Copy link
Member Author

Thanks for the quick reviews, I see no reason to wait with merging this, let's go! 🚀

@infinisil infinisil merged commit cc49e3c into NixOS:master Jul 25, 2024
8 of 9 checks passed
@infinisil infinisil deleted the syntax branch July 25, 2024 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants