Use error instead of *Error in Group
#58
Conversation
|
|
Changes the return value of Group.Wait to error instead of *Error. The *Error concrete type can lead to unintuitive, subtle bugs around nil checks (see: https://golang.org/doc/faq#nil_error). Returning the error interface instead eliminates these. Addresses hashicorp#57.
ccampo133
force-pushed
the
return_error_from_wait
branch
from
f5409ce to
7d7a61a
Compare
|
That would be a breaking change, so unlikely to be accepted. The only solution at this point would be a linter detecting a call to |
| @@ -6,7 +6,7 @@ import "sync" | |||
| // coalesced. | |||
| type Group struct { | |||
| mutex sync.Mutex | |||
| err *Error | |||
| err error | |||
There was a problem hiding this comment.
This change is not necessary.
Instead Wait should return g.err.ErrorOrNil().
There was a problem hiding this comment.
This is a matter of opinion. You could also say using *Error here is unnecessary. Using error is consistent with the API as written in this PR.
| @@ -30,7 +30,7 @@ func (g *Group) Go(f func() error) { | |||
|
|
|||
| // Wait blocks until all function calls from the Go method have returned, then | |||
| // returns the multierror. | |||
| func (g *Group) Wait() *Error { | |||
| func (g *Group) Wait() error { | |||
There was a problem hiding this comment.
Yes. Unfortunately it is a poorly designed API. Release a v2.
In the current version of the API, you are correct. There could certainly be a v2 release though. Unfortunately this is a poorly designed API for the reasons I point out in #57. In fact, I stopped using it years ago, shortly after I ran into this issue. I don't believe there much need for this library now since https://pkg.go.dev/golang.org/x/sync/errgroup exists along with Go 1.20's error joining. |
Changes the type of
Group.errtoerrorinstead of*Error, and also changes the return value ofGroup.Waittoerrorinstead of*Error. The*Errorconcrete type can lead to unintuitive, subtle bugs around nil checks (see: https://golang.org/doc/faq#nil_error). Returning the error interface instead eliminates this possibility. Note that because this changes a public API signature, it should be considered a breaking change.Fixes #57