mage icon indicating copy to clipboard operation
mage copied to clipboard

Support wrapped errors

Open rkennedy opened this issue 10 months ago • 4 comments

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.~~

rkennedy avatar May 01 '24 04:05 rkennedy

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

TotallyGamerJet avatar May 02 '24 02:05 TotallyGamerJet

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.

rkennedy avatar May 02 '24 04:05 rkennedy

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 avatar May 02 '24 11:05 TotallyGamerJet

Thanks for the nudge toward a better solution.

Ofc glad I could help!

TotallyGamerJet avatar May 02 '24 11:05 TotallyGamerJet