Skip to content

Document editorconfig and nixfmt, remove manual rules#330030

Merged
infinisil merged 2 commits into
NixOS:masterfrom
tweag:syntax
Jul 25, 2024
Merged

Document editorconfig and nixfmt, remove manual rules#330030
infinisil merged 2 commits into
NixOS:masterfrom
tweag:syntax

Conversation

@infinisil

Copy link
Copy Markdown
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
Comment thread CONTRIBUTING.md
, argN
}: { }
```
- New files must be formatted by entering the `nix-shell` from the repository root and running `nixfmt`.

@Sigmanificient Sigmanificient Jul 25, 2024

Copy link
Copy Markdown
Member

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

@eclairevoyant eclairevoyant Jul 25, 2024

Copy link
Copy Markdown
Contributor

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

@Sigmanificient Sigmanificient Jul 25, 2024

Copy link
Copy Markdown
Member

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?

@eclairevoyant eclairevoyant Jul 25, 2024

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Member

Choose a reason for hiding this comment

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

Any suggestion then?

@infinisil infinisil Jul 25, 2024

Copy link
Copy Markdown
Member Author

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!

@Sigmanificient Sigmanificient Jul 25, 2024

Copy link
Copy Markdown
Member

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
Copy Markdown
Member

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?

@eclairevoyant eclairevoyant left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
@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