Closed meetme2meat closed 3 years ago
Interesting. I understand the problem (the interface value of error is not a nil interface because we're returning a typed nil from Wrap).
This is a backward incompatible change. That said, I think it would improve the library (and if I were writing it today, it'd probably be like you suggest).
On balance I'm going to merge this and add a section to the README that explains the breakage and how to work around it.
Great Thanks. Let me know when you shell out a new release I'm waiting on it.
it's out already as 1.3.0
@meetme2meat I ended up reverting this because it broke other people's workflows.
Unfortunately that means that the only path forward to fixing this is either a v2, or a separate API that does what you want.
yes, a v2 would be fine I can separate PR around it.
I am currently use code like this (I saved the if code to a template):
// g and h are functions in an external library not in my code.
func g() error {
...
}
func h() error {
...
}
func f() (err error) {
errRaw := g()
if errRaw != nil {
err = errors.Wrap(errRaw, 0)
return
}
errRaw = h()
if errRaw != nil {
err = errors.Wrap(errRaw, 0)
return
}
return
}
This forwards only non nil errors. If error is nil I have no use for wrapping it. I have to check for nil anyway.
You could also use err
directly for errRaw
but for me it is more clear like this.
I think it makes sense to include the behavior in this PR in a version 2. In some cases you do have to check for nil anyway but in other cases it would simplify code if one could write:
func f() error {
// …
return errors.Wrap(g(), 0) // Always return != nil using latest go-errors
}
Especially now that Wrap
and WrapPrefix
includes the guards below it looks like the above code would work even though it won't:
func WrapPrefix(e interface{}, prefix string, skip int) error {
if e == nil {
return nil
}
// …
}
One thing that becomes slightly harder when New
/Wrap
/WrapPrefix
returns error is to access the stack trace. I suggest that a new function errors.ErrorStack(err)
is added to handle this.
Then you could write errors.ErrorStack(err)
where you today write errors.Wrap(err, 0).ErrorStack()
. The nil handling of this function can be discussed. It could be made to only work with non nil input:
// ErrorStack is a convenience function that returns an error's error message
// and callstack of a non nil error. If the provided error has no callstack
// one will be created to the call-site of this function.
func ErrorStack(err error) string {
return Wrap(err, 1).(*Error).ErrorStack()
}
Or it could be made to handle nil by returning an empty string or <nil>
or perhaps <nil>
with a stack trace:
// ErrorStack is a convenience function that returns an error's error message
// and callstack. If the provided error has no callstack, one will be created to
// the call-site of this function. Calling this function with nil is most likely a bug
// but it will return a callstack to this function with the error message `<nil>`.
func ErrorStack(err error) string {
if err == nil {
return New(nil).(*Error).ErrorStack()
}
return Wrap(err, 1).(*Error).ErrorStack()
}
Because of the go gotcha where nil is not nil it may be safest to use the second implementation. One may use the go-errors library by the book but other code may still return a typed nil somewhere which will make your code end up treating a success like and error which may propagate all the way to a central error handler where the stack trace is extracted.
Fix: https://github.com/go-errors/errors/issues/32