hashicorp / go-multierror

A Go (golang) package for representing a list of errors as a single error.
Mozilla Public License 2.0
2.34k stars 124 forks source link

Preserve nil value when no errors have occurred. #8

Open mspiegel opened 8 years ago

mspiegel commented 8 years ago

This patch handles nil values when passed into the slice argument of AppendNonNil(err error, errs ...error). nil values are filtered out and not included in the multierror. When the first argument to AppendNonNil is nil, and the second argument consists entirely of nil values, then return a nil value.

This patch allows the usage the following style.

var result error

result = multierror.AppendNonNil(result, step1())
result = multierror.AppendNonNil(result, step2())

return result

result will be nil if-and-only-if step1() and step2() both return nil.

mspiegel commented 8 years ago

Oh god I had to use reflection. I sympathize if this pull request cannot be accepted. Is there another way to implement this without reflection?

mspiegel commented 8 years ago

I'm beginning to see why this feature was not implemented. The following code would compile fine and then fail at runtime.

var result *multierror.Error

result = multierror.AppendNonNil(result, step1())
result = multierror.AppendNonNil(result, step2())

return result
mitchellh commented 8 years ago

Haha. Yes, I think I had something like this originally and removed it. I can definitely see the use case here though. And yeah, your last example, you definitely would want result to be of type error...

mspiegel commented 8 years ago

Yeah no worries. I can close this pull request. For now I'm just using my fork of go-multierror. Great library BTW this is very useful.

jdonboch commented 7 years ago

what about introducing an AppendNotNil function that simply takes an error instead of ...error... this would be really simple and really useful:

func AppendNonNil(err error, errToAppend error) error {
    if errToAppend == nil {
        return err
    } 
    return Append(err, errToAppend)
}

there may be a better name than AppendNonNil, I can create a PR if that would be acceptable

mspiegel commented 7 years ago

So it's https://golang.org/doc/faq#nil_error that makes the approach challenging. I've since been updating my fork and it now uses reflection to handle all the awful cases. I'm ok with closing this pull request and maintaining my fork. The use cases have sufficiently diverged. OTOH my implementation of func AppendNonNil(err error, errs ...error) error is now fairly robust, so in theory it can be merged...

jdonboch commented 7 years ago

Ah, thanks for the link reference. I was wondering why the method in the PR was so complicated. I am a fan on merging this. Would simplify a lot of code and help reduce the the couple lines of boilerplate if err != nil checks in most code

mspiegel commented 7 years ago

I'm going to replace the Append() function with AppendNonNil() in my fork. To make life simpler for me.

hashicorp-cla commented 5 years ago

CLA assistant check

Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement

Learn more about why HashiCorp requires a CLA and what the CLA includes


1 out of 2 committers have signed the CLA.

Have you signed the CLA already but the status is still pending? Recheck it.