Support wrapped errors #504
Conversation
|
Why not just implement a wrapped error type specifically for mage? That way version compatibility can be maintained |
rkennedy
force-pushed
the
bugfix/wrap-errors
branch
from
4dcf13b to
ca34c2b
Compare
Wrapping errors instead of merely embedding error messages allows calling code to use `errors.Is` and `errors.As` to more precisely check the reasons for various errors instead of having to rely only on substring searches. We implement our own wrapper error to retain backward compatibility prior to Go 1.13, where there is no support for the "%w" format string in `fmt.Errorf`.
rkennedy
force-pushed
the
bugfix/wrap-errors
branch
from
ca34c2b to
549c144
Compare
Well, because my initial reaction was that "just" was doing quite a bit of work in that question! But it turned out not to be so bad after all. I didn't have to do anything crazy like re-implement Thanks for the nudge toward a better solution. |
|
Do we want to actually export the Also, why not make the function signature like this? It improves the calling site. WrapErrorF(err, "error running \"%s %s\": %v\n%s", cmd, strings.Join(args, " "), err, errMsg)
func WrapErrorF(underlying error, format string, args ...interface{}) error {
return &wrappedError{
underlyingError: underlying,
stringError: fmt.Errorf(format, args...),
}
} |
Ofc glad I could help! |
There's no particular reason to make error-wrapping be part of Mage's public interface, so now it's kept in the internal package. One consolation was needed to avoid circular imports, which is that when `mg.F` panics, it does not _wrap_ the JSON-marshalling error.
Instead of having the caller create an error with fmt.Errorf and then pass it to WrapError, we now have WrapErrorf that wraps the error and formats a string error at once.
Wrapping errors instead of merely embedding error messages allows calling code to use
errors.Isanderrors.Asto more precisely check the reasons for various errors instead of having to rely only on substring searches.Fixes #503
Since error-wrapping was only introduced in Go 1.13, accepting this PR would involve dropping support for 1.11 and 1.12 (and even earlier, if the readme's claim of supporting all the way back to 1.7 is still accurate). I realize that may jeopardize this PR because it's definitely not a change to make lightly.