-
-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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
doc: add styleguide on use of with
to CONTRIBUTING.md
#294383
base: master
Are you sure you want to change the base?
Conversation
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.
I think as-is, this is too narrow of a scope (focusing on meta
only). Perhaps this should be phrased to discourage with
in any scope where its use may cause confusion. Clarifying the use in meta
within a couple of bullets is fine to me, as long as we also explain (and focus on) the general policy.
CONTRIBUTING.md
Outdated
|
||
#### 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. |
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.
perhaps link to the nix.dev
documentation here?
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.
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.
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.
That guide can be improved too: https://github.com/NixOS/nix.dev
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.
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.
Co-authored-by: éclairevoyant <848000+eclairevoyant@users.noreply.github.com>
Voicing my opinion as a with-hater, I am against merging this pull request. If the technical merits of banishing |
@AndersonTorres this PR does not "banish" the use of for example, nested |
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.
Discussed in the docs team meeting today.
I don't have much of an opinion here, I'm also not a fan of with
, and don't think it would be problematic to have it in the style guide, but I can also understand @AndersonTorres' arguments.
CONTRIBUTING.md
Outdated
|
||
#### 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. |
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.
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.
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/2024-03-14-documentation-team-meeting-notes-113/41462/1 |
Co-authored-by: Silvan Mosberger <github@infinisil.com>
Co-authored-by: Silvan Mosberger <github@infinisil.com>
Closes #292468
Add a 👍 reaction to pull requests you find important.