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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

clarify wording on args@ default handling #8596

Merged
merged 4 commits into from Jul 19, 2023

Conversation

fricklerhandwerk
Copy link
Contributor

Motivation

I got slightly confused about what this was trying to tell me. I wouldn't have understood quickly if I didn't know it beforehand, and it's likely people would glance over it if it sounds hard to grasp.

Context

We recently discussed #1461 and checking the quality of documentation on that topic prompted an update.

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

  • agreed on idea
  • agreed on implementation strategy
  • tests, as appropriate
    • functional tests - tests/**.sh
    • unit tests - src/*/tests
    • integration tests - tests/nixos/*
  • documentation in the manual
  • documentation in the internal API docs
  • code and comments are self-explanatory
  • commit message explains why the change was made
  • new feature or incompatible change: updated release notes

Priorities

Add 馃憤 to pull requests you find important.

Most importantly use shorter sentences and emphasize the key point that defaults aren't taken into account
@Ericson2314
Copy link
Member

No one sentence per line?

> in
> function {}
> f {}
> ````
>
> will evaluate to an empty attribute set.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
> will evaluate to an empty attribute set.
> and both expressions will evaluate to:
>
> ```nix
> [ 23 {} ]
> ```

@roberth roberth added documentation language The Nix expression language; parser, interpreter, primops, evaluation, etc labels Jun 29, 2023
Copy link
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

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

Approving this, but I would like to get at least in a follow-up PR the rewriting example. That I got the rewrite wrong the first time is all the more clear that this stuff is subtle and its useful to see the correct version!

@fricklerhandwerk
Copy link
Contributor Author

Waiting for @roberth for whether I should add this here.

@roberth
Copy link
Member

roberth commented Jul 19, 2023

Waiting for @roberth for whether I should add this here.

It's very illustrative example I think.
I don't care much about the process, but we should have it and doing it here and now seems easiest.

@fricklerhandwerk fricklerhandwerk enabled auto-merge (squash) July 19, 2023 12:20
@fricklerhandwerk fricklerhandwerk merged commit b017371 into master Jul 19, 2023
13 checks passed
@fricklerhandwerk fricklerhandwerk deleted the fricklerhandwerk-patch-1 branch July 19, 2023 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation language The Nix expression language; parser, interpreter, primops, evaluation, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants