-
Notifications
You must be signed in to change notification settings - Fork 12
Don't swallow text from nested errors #108
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly addresses an issue where the detailed message of Error::Other was being swallowed by thiserror, improving debuggability. The change is correct. I've added a comment suggesting a similar improvement for GetBlobError to ensure consistent error reporting throughout the crate.
|
Before: After: Much more useful. I wasted a fair bit of time running that one down 😭 |
Signed-off-by: John Eckersberg <jeckersb@redhat.com>
f90a6fa to
ec54c83
Compare
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request significantly improves error handling by preventing nested error messages from being swallowed. The changes to the #[error] attributes to include the underlying error details will greatly enhance debuggability. I've included a couple of suggestions to further refine the error messages for the Other variants, making them more direct and less redundant.
| /// Conflicting or missing configuration | ||
| Configuration(Box<str>), | ||
| #[error("error")] | ||
| #[error("other error: {0}")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a general catch-all variant like Other, prefixing the error message with "other error: " can be redundant, as the variant name itself already provides this context. Displaying the wrapped error message directly often leads to cleaner and more precise error reporting.
| #[error("other error: {0}")] | |
| #[error("{0}")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm I would rather explicitly know that the error came from the Other variant just so it's easier to know where it might be coming from.
| #[error("retryable error: {0}")] | ||
| Retryable(Box<str>), | ||
| #[error("error")] | ||
| #[error("other error: {0}")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cgwalters
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! This is the real fix for bootc-dev/bootc#1277 (comment)
Signed-off-by: John Eckersberg jeckersb@redhat.com