Closed astrolox closed 3 years ago
Fixes #42
Thank you for the PR. Up front, this PR unfortunately can't be merged without an associated major version bump (v2). Although the it is a "small change" it is still a breaking change and since we use Go modules and use this library heavily expecting the *Error
type to be returned, this would require a major version bump.
Your reasoning on idiomatic Go is completely valid, however we did make this return value the way we did purposefully and thoughtfully based on our call style. We wanted to avoid (or exactly control) the nil is not nil situation and we commonly initialize an empty *Error
to setup formatting or some other thing and call append on it throughout (potentially with nil errors).
The function you're looking for is probably ErrorOrNil
which is what we always call on return (return merr.ErrorOrNil()
) which returns the stdlib error
type as you expect. This is safe to call on a nil *Error
.
I think if we were building this library from scratch, we may consider changing the semantics as you've proposed. I don't disagree with them. However, in the context of a library that has existed for many years and is already deeply in use in dozens of our own products (and thousands of other downstreams according to pkg.go.dev), I don't think the change justifies the breaking change and major version bump.
No problem. Thank you for the quick reply. I'll maintain a fork for my own use.
Thanks for an excellent library!
I'm opening this pull request to propose a several very small changes which are intended to make the Append function "behave as you would expect" (as is suggested in the README).
Changes:
Return the standard library
error
interface fromAppend
(this is a change to the function signature)Note the following advice from the golang FAQ
Return the standard library
error
interface from(g *Group) Wait()
(this is a change to the function signature)Return
nil
fromAppend
when there is no errorReturn the original error from
Append
when there is only one error (i.e. appending to nil)Introduce a new
Cast
method to allow older code which requires a*multierror.Error
to be easily updatedChange the
Group.err
field from*Error
toerror