Change Error message @checkOverlay #5147
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.
| @@ -334,10 +334,10 @@ struct CmdFlakeCheck : FlakeCommand | |||
| try { | |||
| 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 { ... }.
| @@ -334,10 +334,10 @@ struct CmdFlakeCheck : FlakeCommand | |||
| try { | |||
| 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.
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.