Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrapped errors (github.com/pkg/errors) #28

Closed
fasterthanlime opened this issue Jun 30, 2018 · 5 comments
Closed

Wrapped errors (github.com/pkg/errors) #28

fasterthanlime opened this issue Jun 30, 2018 · 5 comments

Comments

@fasterthanlime
Copy link
Contributor

My fork unwraps errors passed to ErrCode:

https://github.com/fasterthanlime/sqlite/blob/2c204735769c318990e7b4e133a4f770fea28498/error.go#L341-L352

I'm using https://github.com/pkg/errors throughout my codebases for proper stack traces, etc.


It's not the only Go package that allows that, though, so maybe unwrapping should be pluggable?

Something like:

sqlite.SetErrorUnwrapper(errors.Cause)

The signature would be:

type ErrorUnwrapper func (err error) error
var errorUnwrapper ErrorUnwrapper

func SetErrorUnwrapper(eu ErrorUnwrapper) { /* ... */ }

In ErrCode, there'd be a errorUnwrapper != nil check - this should be pretty cheap for folks who don't wrap errors.

What do you think? I know it's not ideal, but I can't think of a better solution right now.

If this approach is chosen, the docs should be very clear that only applications should call this, not libraries, and maybe calling SetErrorUnwrapper a second time should panic.

@fasterthanlime
Copy link
Contributor Author

fasterthanlime commented Jun 30, 2018

Alternative idea:

type causer interface {
  Cause() error
}

func ErrCode(err error) ErrorCode { 
  if ce, ok := err.(causer); ok {
    return ErrCode(ce.Cause())
  }

  // ... current ErrCode code
}

This would support github.com/pkg/errors without creating a dependency.

Just two data points:

@crawshaw
Copy link
Owner

Your second alternative is not so bad, but I wonder if this belongs in the sqlite package. I think a user can write their own ErrCode wrapper function and use that instead of sqlite.ErrCode, which can do any kind of handling they want. Is that not the case?

@fasterthanlime
Copy link
Contributor Author

fasterthanlime commented Jun 30, 2018

Your second alternative is not so bad, but I wonder if this belongs in the sqlite package. I think a user can write their own ErrCode wrapper function and use that instead of sqlite.ErrCode, which can do any kind of handling they want. Is that not the case?

Well, ErrCode() is used in sqliteutil to determine whether to RELEASE or ROLLBACK at the end of a savepoint so.. yes and no.

(The err that Save()'s releaseFn analyzes would definitely be wrapped)

I suppose I could reimplement Save myself, but as we've seen today, it is tricky to get right :)

fasterthanlime added a commit to fasterthanlime/sqlite that referenced this issue Jun 30, 2018
@crawshaw
Copy link
Owner

sqliteutil.Save is a good argument. If you want to send a PR that looks for a causer interface I'll take it.

@crawshaw
Copy link
Owner

Oh you did already. :-)

fasterthanlime added a commit to fasterthanlime/sqlite that referenced this issue Jun 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants