magefile / mage

a Make/rake-like dev tool using Go
https://magefile.org
Apache License 2.0
4.12k stars 251 forks source link

Support wrapped errors #504

Open rkennedy opened 5 months ago

rkennedy commented 5 months ago

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.

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.

TotallyGamerJet commented 5 months ago

Why not just implement a wrapped error type specifically for mage? That way version compatibility can be maintained

rkennedy commented 5 months ago

Why not just implement a wrapped error type specifically for mage? That way version compatibility can be maintained

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 errors.Is, just write a wrapper that implements Error() and Unwrap(), and then add a bunch of calls to use the new error type. The call sites look cumbersome within this library, but consumers of the library should never notice a difference. Using build tags, we can ensure that the new functionality is tested on versions that support it.

Thanks for the nudge toward a better solution.

TotallyGamerJet commented 5 months ago

Do we want to actually export the WrapError function for anyone to use? If not, I'd move it into the internal/ package.

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...),
    }
}
TotallyGamerJet commented 5 months ago

Thanks for the nudge toward a better solution.

Ofc glad I could help!