Add marshal and unmarshal methods for Error.

[theherk]

This is based off #15, which was never merged due to cla.

[hashicorp-cla]

All committers have signed the CLA.

[mentalisttraceur]

For anyone looking at this one, I made a comment in that other earlier pull request that you might be interested in.

[theherk]

For anyone looking at this one, I made a comment in that other earlier pull request that you might be interested in.

You make an excellent point there, @mentalisttraceur. I have reworked my code to do this, since it seems like a much better solution. Unfortunately, after the long holiday torpor, I did some bad git stuff, so I accidentally integrated other changes into this pr. I'm about to fix that up.

[theherk]

Okay, this has been fixed up now. This implementation now differs slightly from #15 base on this comment. I think this is improved. Hope to hear from you @mentalisttraceur, and maybe @mitchellh or a maintainer could get one of these merged.

[josegonzalez]

Is there any chance this (or anything like it) could be merged? I'm hoping to use this in a Nomad API wrapper to send clients more complex errors.

[theherk]

I haven't followed up on this in quite some time, so it looks like there are conflicts now. I'll resolve those soon, though I doubt that will cause it to be merged. I don't know why this has sat so long, but at least that way you could operate from the updated fork with the patch.

[theherk]

This is now up-to-date again, and should be ready for merge.

[josegonzalez]

Heh somehow still seems there is a file conflict? :(

[theherk]

Heh somehow still seems there is a file conflict? :(

Why do you say that? Conflicts are resolved and checks are passing?

[maxiride]

Passing by after encountering the same issue with marshalling multierror.

Is there an estimate for the merge of this PR?

[eculver]

Apologies for all the delays with this one. I am happy to shepherd it along in whatever way possible.

That said, I do have reservations about adding JSON marshal/unmarshal implementations to the library. For one, serializing it as a slice conflicts with the native error type’s singular nature which makes the behavior slightly counter-intuitive.

Also, error values are not directly marshal-able since error is just an interface. This would mean that users would have to start dealing with multierror.Error values instead of just error values. One of the goals of this library is to be unobtrusive meaning users can continue using native error values without ever having to deal with multierror.Error values at all (IMHO this is what makes this library so nice to use). For users to start having to use multierror.Error values directly would violate this goal.

There’s also the issue of custom marshal/unmarshal functionality being difficult to maintain as a library. For example, what happens if today we decide to marshal as an array, but we later decide that, because multierror.Error can contain an arbitrary number of errors, we decide to make it a truncated string like “there were N errors…” or some other form of unique logic. We couldn’t ever make that change because it would not be backwards compatible. The point here is that callers should probably be deciding how their values get marshaled and unmarshaled rather than the library.

And then there’s the fact that if you really want to marshal the value as a slice, then you can use errors.Unwrap to unpack all the errors into a slice of error strings and then marshal that:

// Assume result is a multierror value
result := somefunc()
errs := []string{}

for {
    if err := errors.Unwrap(result); err != nil {
        errs = append(errs, err.Error())
        result = err
        continue
    }
    break
}

mystruct := struct {
    Body   string
    Errors []string
}{
    Body:   "body",
    Errors: errs,
}

bs, err := json.Marshal(&mystruct)
if err != nil {
    panic("marshaling failed")
}
fmt.Print(string(bs))

// Output: {"Body":"body","Errors”:[“…”,”…”,”…”]}

I’m open to discussing it, but for all these reasons, I think we should hold off on adding this functionality and leave it up to the calling code to decide how these values get marshaled and unmarshaled.

[theherk]

@eculver All good points. Too much time has passed for me to still possess strong feelings or need for this feature. If I had a better memory, maybe I could recall my justification better. In any case, if the discussion continues and requires changes, I will happily make them.

[eculver]

Sounds good, I'll go ahead and close this but feel free to re-open if you feel I'm wrong. Thanks for the discussion :)