feat: support std errors functions #213

Sherlock-Holo · 2019-11-03T16:54:12Z

add function Is, As and Unwrap, like std errors, so that we can
continue to use pkg/errors with go1.13 compatibility

add function `Is`, `As` and `Unwrap`, like std errors, so that we can continue to use pkg/errors with go1.13 compatibility Signed-off-by: Sherlock Holo <sherlockya@gmail.com>

Signed-off-by: Sherlock Holo <sherlockya@gmail.com>

update makefile to download dependencies before test anything Signed-off-by: Sherlock Holo <sherlockya@gmail.com>

Signed-off-by: Sherlock Holo <sherlockya@gmail.com>

.gitignore

.travis.yml

errors.go

go.mod

some change are doing by PR pkg#206 and pkg#212 , so I don't need to do it Signed-off-by: Sherlock Holo <sherlockya@gmail.com>

puellanivis

I think you’re building tests upon pure wrappers of external packages, which should not be our responsibility.

go113_test.go

Signed-off-by: Sherlock Holo <sherlockya@gmail.com>

jayschwa · 2019-11-10T16:38:08Z

If this package is going into maintenance mode (according to the README), why add new API surface area? "Maintenance mode" and "drop-in replacement" seem like competing motivations, especially as the standard library beefs up its package.

abraithwaite · 2019-11-11T00:27:53Z

If this package is going into maintenance mode (according to the README), why add new API surface area? "Maintenance mode" and "drop-in replacement" seem like competing motivations, especially as the standard library beefs up its package.

The change is small, it improves the user experience, and probably improves interoperability of tooling as well (less imports, don't have to alias the import). I think it would be silly not to include it personally.

Sherlock-Holo · 2019-11-11T16:20:50Z

@aperezg please see are there anything need to improve, if has, let me know to optimize codes :-)

go113_test.go

go113.go

`Unwrap` just use type assert, it doesn't need go1.13 actually Signed-off-by: Sherlock Holo <sherlockya@gmail.com>

Signed-off-by: Sherlock Holo <sherlockya@gmail.com>

jayschwa · 2019-11-12T16:20:16Z

errors.go

@@ -286,3 +286,13 @@ func Cause(err error) error {
 	}
 	return err
 }
+
+func Unwrap(err error) error {


Why reimplement this function rather than wrapping it? Someone using Go 1.12 or older will not be using the new style of errors. This creates a risk of incompatibility if Go 1.14 subtly changes its Unwrap implementation.

@aperezg emm.... this is a problem if go1.14 changes Unwrap implment, so... we should keep Unwrap >= go1.13?

But why is the problem to maintain the Unwrap function like today? I understand that the Unwrap is an interface on a std library right? Sorry, I try to understand what is the point to change the implementation a wrapped with the standard library. I doubt that they implement a breaking change on that, no?

https://github.com/pkg/errors/blob/master/errors.go#L163

go1.14 cannot break the semantics of Unwrap without violating the go1 compat guarantee.

These same considerations apply to successive point releases. For instance, code that runs under Go 1.2 should be compatible with Go 1.2.1, Go 1.3, Go 1.4, etc., although not necessarily with Go 1.1 since it may use features added only in Go 1.2

This functionality should likely be folded into Cause() and Unwrap() here just calls Cause().

c.f. #215

as that says, 1.14 errors won't change implementation, so we could make unwrap continue in normal package @aperezg

That Unwrap only works for specific permutations of wrapping with the standard library and causers.

In the wild, we should expect any possible permutation of wrapping, including multiple times being wrapped in an alternating manner.

Unwrap should only unwrap error one times, no matter the inner error implement unwrap interface or not, like std Unwrap do.

But this code would fail to fully unwrap: Unwrap(fmt.Errorf("wrap1: %w", myPrivateCauserOnlyError(fmt.Errorf("wrap 3: %w", err), "wrap2:"))) where I would get the fmt.Errorf("wrap3: %w", err) error not the original err, which would not be the expected behavior.

🤦‍♀ the standard library Unwrap does not continuously unwrap a function, until it reaches the end result, unlike how Cause in this package works.

If we implement Unwrap ourselves, may create a risk of incompatibility if Go 1.14 subtly changes its `Unwrap` implementation. <go1.13 users doesn't have `Is` or `As`, if they want, they will use xerrors and it also provides `Unwrap` Signed-off-by: Sherlock Holo <sherlockya@gmail.com>

mfridman · 2019-11-15T20:50:11Z

@jayschwa why return nil instead of the original error that was passed in?

#213 (comment)

I.e., Unwrap may not get a causer or unwrapper, so shouldn't it return the error without modification?

jayschwa · 2019-11-15T20:54:40Z

@jayschwa why return nil instead of the original error that was passed in?

Because we want to match the behavior of the standard library's Unwrap:

Unwrap returns the result of calling the Unwrap method on err, if err's type contains an Unwrap method returning error. Otherwise, Unwrap returns nil.

johan-lejdung · 2019-11-24T12:46:38Z

What's the status on this PR, it's been almost a month since the initiative to update this package started. I'd be happy to help out any way I can.

Sherlock-Holo · 2019-11-24T13:43:47Z

you can tell us if you want these functions only support 113 or all version Johan Lejdung <notifications@github.com> 于 2019年11月24日周日 20:46写道：

…

What's the status on this PR, it's been almost a month since the initiative to update this package started. I'd be happy to help out any way I can. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#213?email_source=notifications&email_token=ACNA6KLNDDD27ZBXC5HOZS3QVJZTBA5CNFSM4JIMD2U2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFAKNTQ#issuecomment-557885134>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACNA6KJ3S7DYMDY2VEO5RN3QVJZTBANCNFSM4JIMD2UQ> .

aperezg · 2019-11-24T14:30:51Z

Sorry I was very busy this month, I'll try to catch up with all comments ASAP

Sherlock-Holo · 2019-11-24T15:14:52Z

@aperezg I think this PR should focus on add this functions for 1.13, and if many<1.13 users want them, we cloud create another PR to help them

johan-lejdung · 2019-11-24T19:10:12Z

I agree with that @Sherlock-Holo

The pr should focus on adding support for the new std errors functionalities. Rebuilding those functionalities for use on <1.13 doesn't make as much sense and isn't as pressing of an issue

ajpetersons · 2019-12-06T13:24:21Z

How soon could this get merged? I would really love to see this PR merged in master and tagged so we can use Go 1.13 functions.

Sherlock-Holo · 2019-12-06T14:18:00Z

these days I am a little busy too…maybe this weekend I can update this pr for just support these functions >=1.13 Artūrs Jānis Pētersons <notifications@github.com> 于 2019年12月6日周五 21:24写道：

…

How soon could this get merged? I would really love to see this PR merged in master and tagged so we can use Go 1.13 functions. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#213?email_source=notifications&email_token=ACNA6KLGVZRYDBMULWRDMV3QXJHAPA5CNFSM4JIMD2U2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGECFXA#issuecomment-562569948>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACNA6KJCDXV33TCZNHPEPBLQXJHAPANCNFSM4JIMD2UQ> .

aperezg · 2019-12-13T08:38:41Z

For me, I think that this PR would be only for add support to the new features on Go 1.13, so I think that as is now seems good.

these days I am a little busy too…maybe this weekend I can update this pr
for just support these functions >=1.13

@Sherlock-Holo What do yo need to update on this PR to be merged?

Furthermore I see that @puellanivis and @jayschwa I see that you also reviewed this PR, is good for you now?

Sherlock-Holo · 2019-12-13T15:36:35Z

@aperezg Nothing need to do before merge, now this PR only add new feature for >= 1.13

Sherlock-Holo added 6 commits November 4, 2019 00:53

feat: support std errors functions

69e8329

add function `Is`, `As` and `Unwrap`, like std errors, so that we can continue to use pkg/errors with go1.13 compatibility Signed-off-by: Sherlock Holo <sherlockya@gmail.com>

style: delete useless comments

5526281

Signed-off-by: Sherlock Holo <sherlockya@gmail.com>

build: update makefile

227a7b4

update makefile to download dependencies before test anything Signed-off-by: Sherlock Holo <sherlockya@gmail.com>

build: fix makefile

9c503b9

Signed-off-by: Sherlock Holo <sherlockya@gmail.com>

chore: delete useless comments

34b42fb

Signed-off-by: Sherlock Holo <sherlockya@gmail.com>

Restore Makefile

057b7e2

aperezg suggested changes Nov 6, 2019

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

.travis.yml Outdated Show resolved Hide resolved

errors.go Outdated Show resolved Hide resolved

errors.go Outdated Show resolved Hide resolved

go.mod Outdated Show resolved Hide resolved

revert: revert some change

d9389f6

some change are doing by PR pkg#206 and pkg#212 , so I don't need to do it Signed-off-by: Sherlock Holo <sherlockya@gmail.com>

puellanivis reviewed Nov 7, 2019

View reviewed changes

go113_test.go Show resolved Hide resolved

go113_test.go Outdated Show resolved Hide resolved

Sherlock-Holo added 3 commits November 9, 2019 19:44

Merge remote-tracking branch 'pkg/master' into support-std-errors

c93d5ce

test: add more check for As unit test

d0d34df

Signed-off-by: Sherlock Holo <sherlockya@gmail.com>

revert: only support Is As Unwrap for >=go1.13

487ff7b

Signed-off-by: Sherlock Holo <sherlockya@gmail.com>

aperezg mentioned this pull request Nov 9, 2019

Add support for Go 1.13 error chains #206

Merged

puellanivis reviewed Nov 11, 2019

View reviewed changes

go113_test.go Show resolved Hide resolved

aperezg reviewed Nov 12, 2019

View reviewed changes

go113.go Outdated Show resolved Hide resolved

Sherlock-Holo added 2 commits November 13, 2019 00:09

feat(Unwrap): allow <go1.13 can use Unwrap

e473c5d

`Unwrap` just use type assert, it doesn't need go1.13 actually Signed-off-by: Sherlock Holo <sherlockya@gmail.com>

test: add go1.13 errors compatibility check

bd678d4

Signed-off-by: Sherlock Holo <sherlockya@gmail.com>

jayschwa reviewed Nov 12, 2019

View reviewed changes

aperezg approved these changes Dec 13, 2019

View reviewed changes

aperezg added this to the 0.9 milestone Dec 13, 2019

aperezg merged commit 6d954f5 into pkg:master Jan 3, 2020

This was referenced Mar 15, 2021

Bump github.com/pkg/errors from 0.8.1 to 0.9.1 wtsi-npg/extendo#52

Merged

Bump github.com/pkg/errors from 0.7.0 to 0.9.1 ktr0731/dependabot-playground#2

Merged

Bump github.com/pkg/errors from 0.7.0 to 0.9.1 ktr0731/dependabot-playground#3

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support std errors functions #213

feat: support std errors functions #213

Sherlock-Holo commented Nov 3, 2019

puellanivis left a comment

jayschwa commented Nov 10, 2019

abraithwaite commented Nov 11, 2019 •

edited

Sherlock-Holo commented Nov 11, 2019

jayschwa Nov 12, 2019

Sherlock-Holo Nov 12, 2019

aperezg Nov 12, 2019

puellanivis Nov 12, 2019

puellanivis Nov 12, 2019

Sherlock-Holo Nov 13, 2019

puellanivis Nov 15, 2019

Sherlock-Holo Nov 15, 2019

puellanivis Nov 16, 2019

puellanivis Nov 16, 2019

mfridman commented Nov 15, 2019

jayschwa commented Nov 15, 2019

johan-lejdung commented Nov 24, 2019

Sherlock-Holo commented Nov 24, 2019 via email

aperezg commented Nov 24, 2019

Sherlock-Holo commented Nov 24, 2019

johan-lejdung commented Nov 24, 2019

ajpetersons commented Dec 6, 2019

Sherlock-Holo commented Dec 6, 2019 via email

aperezg commented Dec 13, 2019 •

edited

Sherlock-Holo commented Dec 13, 2019

feat: support std errors functions #213

feat: support std errors functions #213

Conversation

Sherlock-Holo commented Nov 3, 2019

puellanivis left a comment

Choose a reason for hiding this comment

jayschwa commented Nov 10, 2019

abraithwaite commented Nov 11, 2019 • edited

Sherlock-Holo commented Nov 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfridman commented Nov 15, 2019

jayschwa commented Nov 15, 2019

johan-lejdung commented Nov 24, 2019

Sherlock-Holo commented Nov 24, 2019 via email

aperezg commented Nov 24, 2019

Sherlock-Holo commented Nov 24, 2019

johan-lejdung commented Nov 24, 2019

ajpetersons commented Dec 6, 2019

Sherlock-Holo commented Dec 6, 2019 via email

aperezg commented Dec 13, 2019 • edited

Sherlock-Holo commented Dec 13, 2019

abraithwaite commented Nov 11, 2019 •

edited

aperezg commented Dec 13, 2019 •

edited