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.

Sign up for GitHub

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

Jump to bottom

doc: add styleguide on use of with to CONTRIBUTING.md #294383

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

doc: add styleguide on use of with to CONTRIBUTING.md #294383

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
aab7baf
doc: add styleguide on use of `with` to CONTRIBUTING.md
lolbinarycat Mar 9, 2024
8b27943
CONTRIBUTING.md: fix typo
lolbinarycat Mar 11, 2024
0e45fe7
Update CONTRIBUTING.md
lolbinarycat Mar 14, 2024
e3c0fdb
Update CONTRIBUTING.md (fix typo)
lolbinarycat Mar 14, 2024
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
20 changes: 20 additions & 0 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -740,3 +740,23 @@ Names of files and directories should be in lowercase, with dashes between words

As an exception, an explicit conditional expression with null can be used when fixing a important bug without triggering a mass rebuild.
If this is done a follow up pull request _should_ be created to change the code to `lib.optional(s)`.

#### Use of `with`

`with` is a somewhat controversial feature of nix syntax, it can reduce repition, but easily can make code unreadable or buggy if used carelessly.
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps link to the nix.dev documentation here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i considered it, but honestly, i don't think very highly of that guide, as it frankly lacks a lot of nuance, and feels like it might as well call with a deprecated feature. it doesn't provide any examples of times it would be ok or even encouraged to use.

Copy link
Contributor

Choose a reason for hiding this comment

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

That guide can be improved too: https://github.com/NixOS/nix.dev

Copy link
Member

Choose a reason for hiding this comment

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

I think it would be great to improve the article on with with acceptable uses and a better explanation of the problems. We can also link it from here for completeness already though.

I think this contributing section here is still valuable since it's more Nixpkgs specific and doesn't necessarily apply to Nix in general.

lolbinarycat marked this conversation as resolved.
Show resolved Hide resolved

recommended uses:
- `meta.maintainers = with lib.maintainers; [ ... ]`
- for packages with **multiple** licenses: `meta.license = with lib.licenses; [ ... ]`

acceptable uses:
- `meta = with lib; { ... }`
this allows you to not have to write `lib` twice for `licenses` and `maintainers`, at the cost of nesting `with`.

unacceptable uses:
- `with lib;` anywhere outside of `meta`
- nested `with` anywhere outside of `meta`
- `with` at the top of a file
- `with` at the start of an expression that spans
lolbinarycat marked this conversation as resolved.
Show resolved Hide resolved

uses of `with` other than those listed here are left up to the discretion of PR reviewers.
Loading