Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preserve nil value when no errors have occurred. #8

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

mspiegel
Copy link

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 and others added 5 commits August 10, 2016 21:43
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
Copy link
Author

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?

Do not convert 'nil' error to 'nil' *Error.
@mspiegel
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

mspiegel commented Aug 11, 2016

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
Copy link

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
Copy link
Author

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
Copy link

jdonboch commented Nov 16, 2016

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
Copy link
Author

mspiegel commented Mar 9, 2017

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

@hashicorp-cla
Copy link

hashicorp-cla commented Mar 26, 2019

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.

  • sethvargo
  • mspiegel

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants