Change Error message @checkOverlay#5147
Change Error message @checkOverlay#5147bryanhonof wants to merge 2 commits intoNixOS:masterfrom bryanhonof:master
Conversation
The current error message makes it look like something went wrong during parsing of the flake. Whilst it's just a new convention to use `final: prev: ...` instead of `self: super: ...`. This change makes it more clear what the user did wrong.
| state->forceValue(v, pos); | ||
| if (!v.isLambda() || v.lambda.fun->matchAttrs || std::string(v.lambda.fun->arg) != "final") | ||
| throw Error("overlay does not take an argument named 'final'"); | ||
| throw Error("By convention the attribute 'overlay' expects the argument 'final' not '" + std::string(v.lambda.fun->arg) + "'"); |
There was a problem hiding this comment.
This message isn't right for "matchAttrs" type lambdas, such as { foo, bar }: x. That case should have its own message.
Same below.
There was a problem hiding this comment.
@roberth Do you have a proposal for what error it should throw?
There was a problem hiding this comment.
By convention the attribute 'overlay' is expected to be a simple lambda starting with 'final: ', not an attribute set matching lambda.
I guess you could add a representation of it too, but a source position Pos would be even better.
There was a problem hiding this comment.
Position info is already added to the error context by the try { ... } catch { ... }.
| state->forceValue(v, pos); | ||
| if (!v.isLambda() || v.lambda.fun->matchAttrs || std::string(v.lambda.fun->arg) != "final") | ||
| throw Error("overlay does not take an argument named 'final'"); | ||
| throw Error("By convention the attribute 'overlay' expects the argument 'final' not '" + std::string(v.lambda.fun->arg) + "'"); |
There was a problem hiding this comment.
This will crash if v is not a lambda.
There was a problem hiding this comment.
In the new commit the !v.isLambda() check is now isolated, and also throws a more appropriate error message, telling the user the attribute should be a lambda.
| if (!v.isLambda()) | ||
| throw Error("Expected a lambda for the overlay attribute, but got something else"); | ||
| if (v.lambda.fun->matchAttrs) | ||
| throw Error("By convention the attribute 'overlay' is expected to be a simple lambda starting with 'final: ', not an attribute set matching lambda"); |
There was a problem hiding this comment.
| throw Error("By convention the attribute 'overlay' is expected to be a simple lambda starting with 'final: ', not an attribute set matching lambda"); | |
| throw Error("the attribute 'overlay' must a function that takes a single argument named 'final ', not an attribute set"); |
It's not a convention but a requirement. Also instead of "lambda" it's better to say "function".
There was a problem hiding this comment.
What is responsible for this "requirement"?
Assuming we do need this, why say "function", a general term, when the value must be provided by a lambda? Or does Nix have another implementation of "function" that is acceptable here, but is not a lambda?
There was a problem hiding this comment.
This exact check is the requirement.
|
I marked this as stale due to inactivity. → More info |
fricklerhandwerk
added
documentation
UX
Some of these are a bit after the specified time period, but I also believe some of these items are difficult to gauge how much "impact" they had. Helped organize, host, and run, the Summer of Nix Lecture Series 2022 - https://www.youtube.com/playlist?list=PLt4-_lkyRrOMWyp5G-m_d1wtTcbBaOxZk - NixOS/infra#213 Helped organize, and host infra for, the Summer of Nix Lecture Series 2023 - https://www.youtube.com/playlist?list=PLt4-_lkyRrOPcBuz_tjm6ZQb-6rJjU3cf - NixOS/infra#240 Helped organize, host, and was responsible for livestreaming infra during, NixCon Paris 2022 - https://www.youtube.com/playlist?list=PLgknCdxP89ReD6gxl755B6G_CI65z4J2e Maintenance of some nixpkgs packages - NixOS/nixpkgs#340223 (contribution after 2024-05-01) - NixOS/nixpkgs#290084 - NixOS/nixpkgs#170089 Organized, and assembled a team for the FOSDEM 2023 Nix/NixOS Devroom - https://discourse.nixos.org/t/fosdem-2023-nix-and-nixos-devroom/23133 Organizer & sole maintainer of the Config Management Camp Nix track - https://discourse.nixos.org/t/config-management-camp-2023-ghent/23455 - https://discourse.nixos.org/t/config-management-camp-2024-ghent/33852 - https://discourse.nixos.org/t/cfgmgmtcamp-2025-is-looking-for-nix-presentations/51658 (contribution after 2024-05-01) Public speaking & spreading awareness of Nix/NixOS - https://youtu.be/gUjvnZ9ZwMs?si=nDiZTCpQj53wwq8P - https://www.youtube.com/watch?v=hNcYPH5Q_pA&t=862s The occasional dabble into the Nix C++ code base - NixOS/nix#11494 (contribution after 2024-05-01) - NixOS/nix#11490 (contribution after 2024-05-01) - NixOS/nix#11489 (contribution after 2024-05-01) - NixOS/nix#11349 (contribution after 2024-05-01) - NixOS/nix#11241 (contribution after 2024-05-01) - NixOS/nix#9557 - NixOS/nix#8788 - NixOS/nix#8212 - NixOS/nix#5147 General evangelism Pretty much every event I attend, I'm talking about Nix, showing off Nix/NixOS, and just trying to get people to see how awesome this tool is.
4 participants
The current error message makes it look like something went wrong during parsing of the flake. Whilst it's just a new convention to use
final: prev: ...instead ofself: super: .... This change makes it more clear what the user did wrong.See #5146.