mgmt icon indicating copy to clipboard operation
mgmt copied to clipboard

Use fmt.Errorf and replace errwrap.Wrapf

Open frebib opened this issue 4 years ago • 5 comments

Go 1.13 adds %w to fmt.Errorf which is a first-party replacement for the mgmt errwrap package. https://blog.golang.org/go1.13-errors

The argument ordering differs slightly, but porting it should be entirely doable.

frebib avatar Feb 07 '21 16:02 frebib

I think the simplest form of this change would be as follows

diff --git a/util/errwrap/errwrap.go b/util/errwrap/errwrap.go
index 3bd64e3..6c1ac2f 100644
--- a/util/errwrap/errwrap.go
+++ b/util/errwrap/errwrap.go
@@ -19,14 +19,15 @@
 package errwrap
 
 import (
+	"fmt"
+
 	"github.com/hashicorp/go-multierror"
-	"github.com/pkg/errors"
 )
 
 // Wrapf adds a new error onto an existing chain of errors. If the new error to
 // be added is nil, then the old error is returned unchanged.
 func Wrapf(err error, format string, args ...interface{}) error {
-	return errors.Wrapf(err, format, args...)
+	return fmt.Errorf("%s: %w", fmt.Sprintf(format, args), err)
 }
 
 // Append can be used to safely append an error onto an existing one. If you

Although it could certainly be taken one further to inline the usage of the Wrapf function and then remove it completely as it's redundant

frebib avatar Feb 07 '21 19:02 frebib

What's the current reason of existence for errors.Wrapf? Was it only because of golang didn't support it until now?

If those errors.Wrapf were only to add into error chain, I believe it would indeed be better for readability to use fmt.Errorf %w. The intent is (by far) more clear for me that way.

evrardjp avatar Mar 03 '21 12:03 evrardjp

On Wed, Mar 3, 2021 at 7:20 AM Jean-Philippe Evrard [email protected] wrote:

What's the current reason of existence for errors.Wrapf? Was it only because of golang didn't support it until now?

Correct.

If those errors.Wrapf were only to add into error chain, I believe it would indeed be better for readability to use fmt.Errorf %w. The intent is (by far) more clear for me that way.

I think we should probably switch over, however the problem is that we can do return errwrap.Wrapf(err, "whatever") and if err is nil, the whole thing is nil.

purpleidea avatar Mar 03 '21 19:03 purpleidea

Let's write a bunch of if err != nil then!

I don't mind having a look at this. @frebib , can I use your help on this? I hope to reduce code/tech debt by removing the whole errwrap in a decent refactor.

evrardjp avatar Mar 03 '21 21:03 evrardjp

Sure thing @evrardjp, anything you need. I did try implementing this in the naive way and it backfired, presumably because the nils

frebib avatar Mar 03 '21 22:03 frebib