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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better node errors using error messages in output types #2445

Merged
merged 2 commits into from
Jan 12, 2024

Conversation

RunDevelopment
Copy link
Member

This PR adds the ability for output types to return error messages. This is useful for complex types where never_reason alone isn't enough. Since never_reason is only one message, we previously weren't able to communicate the precise reason for the error, even though the type system knows the reason. I fixed this by allowing output types to return not only their type, but also an optional Error { message: string } type. This allows us to define relevant error messages, instead of just listing reasons why the node could be invalid.

In actual output type code, this looks like this (Alpha Matting node):

if fg <= bg {
    error("The foreground threshold must be greater than the background threshold.")
} else if bool::or(image.width != trimap.width, image.height != trimap.height) {
    error("The image and trimap must have the same size.")
} else {
    Image { width: image.width, height: image.height }
}

image

Note that this PR doesn't remove never_reason. never_reason is just simpler to use in many cases, and it still provides a useful fallback/default for error messages. I think of the new error("...") as an addition, not as a replacement.

@joeyballentine
Copy link
Member

LGTM

@joeyballentine joeyballentine merged commit 6f2d3aa into chaiNNer-org:main Jan 12, 2024
20 checks passed
@RunDevelopment RunDevelopment deleted the better-erros branch January 12, 2024 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants