Skip to content

Change Error message @checkOverlay #5147

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.

Sign up for GitHub

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

Closed
bryanhonof wants to merge 2 commits into from
Closed

Change Error message @checkOverlay #5147

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9e9a024
Change Error message @checkOverlay
bryanhonof Aug 17, 2021
f0fc2a1
Update error messages
bryanhonof Aug 31, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/nix/flake.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) + "'");

@roberth roberth Aug 30, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This message isn't right for "matchAttrs" type lambdas, such as { foo, bar }: x. That case should have its own message.
Same below.

@bryanhonof bryanhonof Aug 30, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@roberth Do you have a proposal for what error it should throw?

@roberth roberth Aug 30, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@edolstra edolstra Aug 30, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Position info is already added to the error context by the try { ... } catch { ... }.

@edolstra edolstra Aug 30, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This will crash if v is not a lambda.

@bryanhonof bryanhonof Aug 31, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

auto body = dynamic_cast<ExprLambda *>(v.lambda.fun->body);
if (!body || body->matchAttrs || std::string(body->arg) != "prev")
throw Error("overlay does not take an argument named 'prev'");
throw Error("By convention the attribute 'overlay' expects the argument 'prev' not '" + std::string(body->arg) + "'");
// FIXME: if we have a 'nixpkgs' input, use it to
// evaluate the overlay.
} catch (Error & e) {
Expand Down