Add recipe: Emit an evaluation warning only once#1170
Add recipe: Emit an evaluation warning only once#1170roberth wants to merge 1 commit intoNixOS:masterfrom
Conversation
hsjobeki
left a comment
hsjobeki
left a comment
There was a problem hiding this comment.
Overall nice document that teaches the important bits and pieces.
The main issues are passive voice, meta-commentary, hedging language, and a few consistency problems.
Not sure if we need the "best of both worlds" section, since it seems a bit nieche
| } | ||
| ``` | ||
|
|
||
| ## Best of both worlds |
There was a problem hiding this comment.
I would drop this. Most people don't need this.
| ## Working principle | ||
|
|
||
| Nix evaluates the root value of an expression file only once. | ||
| We will exploit this property to emit fewer warnings. |
There was a problem hiding this comment.
No future tense please. We are going to add a technical writing guide. Use present tense to describe how things happen.
|
|
||
| The Nix language provides a `warn` function that lets expression authors provide feedback to their code's callers/users. | ||
| Since such warnings are tied to the flow of evaluation, it may happen that the warning you wish to add is trigger too many times. | ||
| This document shows a technique to solve that problem, and documents its requirements and limitations. |
There was a problem hiding this comment.
Pure meta-commentary. I would cut it. Replace with a one-sentence statement of the technique itself or drop entirely. The document works without it.
| # Emit an evaluation warning only once | ||
|
|
||
| The Nix language provides a `warn` function that lets expression authors provide feedback to their code's callers/users. | ||
| Since such warnings are tied to the flow of evaluation, it may happen that the warning you wish to add is trigger too many times. |
There was a problem hiding this comment.
Simplify and fix typo
| Since such warnings are tied to the flow of evaluation, it may happen that the warning you wish to add is trigger too many times. | |
| Since such warnings are tied to the flow of evaluation the warning may fire more times than intended. |
| <...> # The rest of the code | ||
| ``` | ||
|
|
||
| Unfortunately, this means we don't have access to the `lib.warn` family of functions, but fortunately we can usually rely on `builtins.warn` nowadays (more on that later). |
There was a problem hiding this comment.
Don't hedge a fact that has a precise answer. builtins.warn was introduced in a precise version. This sentence adds vagueness.
As a user i would not know if i can use builtins.warn or not.
Sugggestion: drop the nowadays (more on that later) and state facts instead.
| ## `builtins.warn` availability | ||
|
|
||
| `builtins.warn` was introduced in Nix 2.23. | ||
| Most users have upgraded far beyond 2.22, but if your code is in the upgrade path for users who may not have, use this polyfill: |
There was a problem hiding this comment.
Just state the polyfill.
| Most users have upgraded far beyond 2.22, but if your code is in the upgrade path for users who may not have, use this polyfill: | |
| builtins.warn was introduced in Nix 2.23. If you support older versions, use this polyfill: |
Most users have upgraded far beyond 2.22, statement Is not necessary/verified.
Depends who your users are. Most of open source users? Most nixpkgs users? The sentence works without that claim.
|
|
||
| This will cause each `makeWidgetScript` invocation to emit a sizable message, every time it is invoked in the old way. | ||
|
|
||
| Now let's use a top level scope to reduce our warnings to just one. |
There was a problem hiding this comment.
Remove filler intro
| Now let's use a top level scope to reduce our warnings to just one. | |
| Add a top-level ... |
| # ... | ||
| ``` | ||
|
|
||
| The `. or` operator will take care of the potentially missing `warn`. |
There was a problem hiding this comment.
will take is future tense. . or typo
| The `. or` operator will take care of the potentially missing `warn`. | |
| The `or` operator takes care of the potentially missing `warn`. |
2 participants
A technique to reduce warning noise, even for ones with a lot of prosaic context.
This isn't a particularly beginners' topic, but I don't see why we shouldn't have recipes for things you don't immediately run into. I hope it's quite accessible anyway.
Real world example