-
-
Notifications
You must be signed in to change notification settings - Fork 202
feat(errors): ErrorModel implement errors.Unwrap #777
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
base: main
Are you sure you want to change the base?
Conversation
Allow compatibility for errors returned by huma.NewError* with the std' errors.As/errors.Is
18555a5
to
814bc11
Compare
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.
Pull Request Overview
This PR enhances the ErrorModel to support error unwrapping in order to be compatible with std errors.As/errors.Is.
- Adds an unexported field "errors []error" to hold wrapped errors.
- Implements the Unwrap() method to return the slice of errors.
- Updates the Add() and NewError() functions to populate this new field.
Comments suppressed due to low confidence (1)
error.go:97
- Consider using a pointer receiver for the Unwrap method (i.e., func (e *ErrorModel) Unwrap() []error) to maintain consistency with other methods like Error() and Add(), ensuring that modifications to the ErrorModel are consistently observed.
func (e ErrorModel) Unwrap() []error { return e.errors }
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #777 +/- ##
=======================================
Coverage 93.10% 93.11%
=======================================
Files 23 23
Lines 5296 5301 +5
=======================================
+ Hits 4931 4936 +5
Misses 313 313
Partials 52 52 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@@ -107,8 +112,11 @@ func (e *ErrorModel) Error() string { | |||
// Value: 5 | |||
// }) | |||
func (e *ErrorModel) Add(err error) { | |||
e.errors = append(e.errors, err) |
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.
This is great, but huma.NewError
doesn't actually call Add
on the error when constructing the details, and the same is potentially the case when manually returning error models in other code (and handlers in your service). I wonder if there is a better way to support most uses instead of requiring Add
to get called 🤔
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.
Maybe smth like errors.Joins on already existing .Errors
field ?
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.
Maybe a simple edit to the nvm it's already published with such changeNewError
method would already help a bit
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.
Hehe I think we're good !
My changes were for some code that did some stuff like return nil, huma.Error500...("error", ErrorSpecialType{err})
. Turn out my "middleware" tests pass if I change it to smth like return nil, ErrorSpecialType{huma.Error500...("error", err)
Allow compatibility for errors returned by huma.NewError* with the std' errors.As/errors.Is