pkg / errors

Simple error handling primitives
https://godoc.org/github.com/pkg/errors
BSD 2-Clause "Simplified" License
8.18k stars 691 forks source link

Add support for Go 1.13 error chains #206

Closed jayschwa closed 4 years ago

jayschwa commented 5 years ago

Go 1.13 adds support for error chains to the standard libary's errors package. The new standard library functions require an Unwrap method to be provided by an error type. This change adds a new Unwrap method (identical to the existing Cause method) to the unexported error types.

knz commented 5 years ago

I was about to open exactly the same PR. Thank you! We would love this to go through as it will enable https://github.com/cockroachdb/errors to remain backward-compatible with pkg/errors.

sagikazarmark commented 5 years ago

I wonder if it makes sense to add unwrapping to the Cause function as well so that it can unwrap Go 1.13 errors as well. It would automatically make old code using Cause work with Unwrap and give time to transition to Unwrap.

knz commented 5 years ago

that sounds reasonable but perhaps a different PR?

sagikazarmark commented 5 years ago

Well, the title says Add support for Go 1.13 error chains, so for me it belongs here.

jayschwa commented 5 years ago

Unwrapping Go 1.13 error chains can be done with the new functions in the standard library, so I don't see a need to support it here. Perhaps I could have named the PR better, but I'm going to leave it as-is unless I hear differently from Dave.

sagikazarmark commented 5 years ago

Unwrapping Go 1.13 error chains can be done with the new functions in the standard library

Existing code using errors.Cause won't be able to unwrap errors. I think it would be a great forward compatible migration path, if it could.

natefinch commented 5 years ago

IMO this is necessary so that existing codebases can gracefully transition to the stdlib error wrapping without having to go whole-hog changing thousands of lines of code to use UnWrap() rather than Cause. Please take a look, @davecheney

slomek commented 4 years ago

What about using Unwrap() instead of Cause() for unwrapping as well? This would add the compatibility the other way round, so if eg. I'm using errors.Cause(...) in my application, but the inner library starts using Go 1.13 errors chain, I can still extract the root error?

aperezg commented 4 years ago

We're forked this library and modify for using internally new package errors on go 1.13, furthermore on if you use a previous version the library was been compiled about xerrors package.

If anyone want use it our version, will can find it here: https://github.com/friendsofgo/errors

Any feedback is more than welcome!

knz commented 4 years ago

We've made a more extensive errors library that's both compatible with xerrors and pkg/errors and adds a lot of bonus features, see https://github.com/cockroachdb/errors

sagikazarmark commented 4 years ago

I created a drop-in replacement for both the stdlib errors package and pkg/errors: https://github.com/emperror/errors

chuckha commented 4 years ago

I believe this PR is very much needed as the Frame capture did not make it through from xerrors into go1.13. github.com/pkg/errors still has a lot of utility and given the choice between using only one error package, the standard library's error package is less useful than this one.

slomek commented 4 years ago

@chuckha You don't really need the change here to start using Go 1.13 errors, I've describe a workaround on my blog: https://mycodesmells.com/post/migrating-pkg-errors-to-go-113-errors :wink:

Songmu commented 4 years ago

Go 1.13 has been released and Is and As func is introduced in standard error package.

Once this pull request to be merged, we can use errors.As and errors.Is for withMessage and withStack. This is very useful for standard error package and pkg/errors interoperability.

I hope this pull request will be merged soon.

davecheney commented 4 years ago

Thank you for this PR. I'm sorry I have had no time to work on this package since I called for the 1.0 release earlier in the year -- my day job is very demanding. I wasn't expecting that Go 1.13 would ship without stack trace support in its errors. I hope to have time to review this properly in November.

Sherlock-Holo commented 4 years ago

I think if you add Is As Unwrap functions, we can keep using pkg/errors with the new functions. For now, with this PR, we still need to

import (
    stderrors "errors"

    "github.com/pkg/errors"
)

to use Is As Unwrap functions

Sherlock-Holo commented 4 years ago

some error variable in std library (example), after go1.13, can not use == to check simply, need to use errors.Is so add Is As Unwrap function into pkg/errors too is a good idea

Songmu commented 4 years ago

I think we only need pkg/errors.Unwrap, andIs and As aren't needed. We can use standard errors.Is/As.

Songmu commented 4 years ago

The pkg/errors will no longer be needed if the standard package will provide a stack feature. I think that is the desired future.

I think support only Unwrap is necessary for the interoperability and ease of migration between pkg/errors and standard errors, but I don't think other features is necessary.

Sherlock-Holo commented 4 years ago

@Songmu if pkg/errors don't support Is As Unwrap, we need to import stderrors "errors" and it doesn't look good: why I used an error package but still need another one to compare error

aperezg commented 4 years ago

@Sherlock-Holo I'm agree with you that if the package errors had implemented the functions Is and As would be better for don't import two packages at the same time. But if we adding this functions to this package we have three options, one is adding with xerrors package for compatibility with previous version of v1.13, other is using the building tags, and use xerrors and standard and the last is implemented by the pkg/errors.

But in this case I think that this PRs is only for add the compatibility with the standard library, and adding only the Unwrap was enough. If you want, you can add a new issue related with this PRs for discuss if util for this package implement the functions Is and As

Sherlock-Holo commented 4 years ago

@aperezg good idea, I will create a PR to add Is As Unwrap support. Actually I forked this repo and added these functions last night. Let me make it good enough to create a PR

johan-lejdung commented 4 years ago

I'm really looking forward to this PR getting merged 🙏Nice work everyone!

johan-lejdung commented 4 years ago

Consider making a release with these changes, it doesn't seem like there has been a release since 5th of January

aperezg commented 4 years ago

@johan-lejdung Yes, the idea is finish to check #213 (for conclude the support to the 1.13 errors) and see if another pending PR could be approved, and check the Milestone to v0.9.0 and then publish, we try this ASAP on this month, it is possible

johan-lejdung commented 4 years ago

Would it make sense to also extend the Is and As methods in this package?

The only reason I ask is because I've been able to include this package instead of the "errors" package in the past. But as of this change I have to include both packages in order to make use of the Is and As methods. Because the name of the packages are the same it because slightly more cumbersome.

It's not the end of the world, but if this is thought of as a drop-and-replace package I think it would make sense.

aperezg commented 4 years ago

@johan-lejdung there a PR with the method Is and As you can follow his process here: #213.

jasonkeene commented 4 years ago

@aperezg Any chance we could get a tagged release for this? 🙏 Would be nice to no longer use a fork.