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
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
131 changes: 2 additions & 129 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -557,138 +557,11 @@ Names of files and directories should be in lowercase, with dashes between words

### Syntax

- Use 2 spaces of indentation per indentation level in Nix expressions, 4 spaces in shell scripts.

- Do not use tab characters, i.e. configure your editor to use soft tabs. For instance, use `(setq-default indent-tabs-mode nil)` in Emacs. Everybody has different tab settings so it’s asking for trouble.
- Set up [editorconfig](https://editorconfig.org/) for your editor, such that [the settings](./.editorconfig) are automatically applied.

- Use `lowerCamelCase` for variable names, not `UpperCamelCase`. Note, this rule does not apply to package attribute names, which instead follow the rules in [package naming](./pkgs/README.md#package-naming).

- Function calls with attribute set arguments are written as

```nix
foo {
arg = <...>;
}
```

not

```nix
foo
{
arg = <...>;
}
```

Also fine is

```nix
foo { arg = <...>; }
```

if it's a short call.

- In attribute sets or lists that span multiple lines, the attribute names or list elements should be aligned:

```nix
{
# A long list.
list = [
elem1
elem2
elem3
];

# A long attribute set.
attrs = {
attr1 = short_expr;
attr2 =
if true then big_expr else big_expr;
};

# Combined
listOfAttrs = [
{
attr1 = 3;
attr2 = "fff";
}
{
attr1 = 5;
attr2 = "ggg";
}
];
}
```

- Short lists or attribute sets can be written on one line:

```nix
{
# A short list.
list = [ elem1 elem2 elem3 ];

# A short set.
attrs = { x = 1280; y = 1024; };
}
```

- Breaking in the middle of a function argument can give hard-to-read code, like

```nix
someFunction { x = 1280;
y = 1024; } otherArg
yetAnotherArg
```

(especially if the argument is very large, spanning multiple lines).

Better:

```nix
someFunction
{ x = 1280; y = 1024; }
otherArg
yetAnotherArg
```

or

```nix
let res = { x = 1280; y = 1024; };
in someFunction res otherArg yetAnotherArg
```

- The bodies of functions, asserts, and withs are not indented to prevent a lot of superfluous indentation levels, i.e.

```nix
{ arg1, arg2 }:
assert system == "i686-linux";
stdenv.mkDerivation { /* ... */ }
```

not

```nix
{ arg1, arg2 }:
assert system == "i686-linux";
stdenv.mkDerivation { /* ... */ }
```

- Function formal arguments are written as:

```nix
{ arg1, arg2, arg3 }: { /* ... */ }
```

but if they don't fit on one line they're written as:

```nix
{ arg1, arg2, arg3
, arg4
# Some comment...
, 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?


- Functions should list their expected arguments as precisely as possible. That is, write

Expand Down