-
Notifications
You must be signed in to change notification settings - Fork 4
scheme.go: Wrap all errors with context #13
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: there are some strings where we use %v
when we could use %s
, I think. But that shouldn't matter that much.
} | ||
|
||
defer r.Close() | ||
|
||
err = rs.toBkt.Upload(ctx, objectName, r) | ||
if err != nil { | ||
return err | ||
return fmt.Errorf("upload %v to target bucket: %w", objectName, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this all should be errors.Errorf
I think 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may be totally misunderstanding but this doesn’t sound like that to me https://blog.golang.org/go1.13-errors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wooow, this is more complex then I expected. Looks like "github.com/pkg/errors"
and errors
are literally incompatible now. Also just learn that if you use
"github.com/pkg/errors.Wrap"`` and then on top caller you log this and if you use %s
it's different then if you use `%v`... for %v it is stack trace even 0.o
So yea, you are right I guess - let's choose one and use it consistently...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually this should fix the compatibility: pkg/errors#206
@bwplotka @metalmatze @squat @kakkoyun