Proposal: Error aggregation
mantzas opened this issue · 15 comments
It would be nice if we could have a aggregate error struc for handling and propagating multiple errors.
Internally it would have a list of errors, which can be appended.
This struct has only to implement the Error() interface in order to be a error.
The following signatures could be supported:
Append(err error) // in order to append errors to the aggregate
Count() int // in order to provide a way to check if errors exist
Error() string // in order to implement the Error interface
If one needs to return an aggregated list of errors, then one can just return a slice of errors.
Append() would be unnecessary because there is already the generic append(), Count() would likewise be unnecessary because of len().
But you would not have one Error interface implementation which is needed in order to be a error...
The above would not be something that is a error (by interface)
type errorSlice []error
func (e errorSlice) Error() string {
…
}
that would do the trick too.
Error() has to loop through all errors and create one string.
It would be a nice addition to this package.
one small problem though.
There has to be a mutex around Appending(), Count() and Error()...
type errorSlice []error
func (e errorSlice) Error() string {
var s []string
for _, err := range e {
s = append(s, err.Error())
}
return strings.Join(s, ": ")
}
There is no need to put a mutex around it, because it’s local to your own package. If you know you might need to append from multiple goroutines, then one can always mutex lock oneself.
No one should be calling Error until after it has been returned and converted into the error interface, and no one should be calling Count or Append because again, as noted, those functions are unnecessary.
The above 8 lines of code are all one needs for what you want.
I think we should make this type "concurrent" safe in order to avoid having the user to do that, like the Map in the sync package.
As for the error creation i went the strings.Builder way...
I had something like this in mind!
https://github.com/mantzas/patron/blob/develop/errors/aggregate.go
You are mistaking my input. I am not suggesting that this become a feature of this library.
It is 8 lines of code that any package can write. In that case it is simple, clean, elegant.
If we make it generic, concurrent, and cumbersome, we are helping no one.
OBTW for the code linked:
[strings.Builder].Write always returns len(p), nil.
Is that not the purpose of a package? Making something that can be reused, not copied, generic. If it is concurrent on top of it it would be really nice.
Thanks for the strings.Builder... bad API if you ask me... i should not have to drill into the documentation to find out that the err is always nil.
A package should be a clean provider of functionality that provides benefit to the caller. Building an Aggregate from your package is exactly the same amount of code and complexity required for me to use a local []error.
By reusing your code, my code would not be any simpler.
bad API
It must implement the io.Writer interface.
Not having to implement this in all the projects again and again, and provide on top of it some features (concurrency safety) does benefit the caller. This is the reason that i have this in my framework. I thought that someone would benefit from it like i do, and remove my error package from the framework.
Yes i know "a little copying is better than a dependency" but like i said i had to copy this to all over...
That would be almost 50 lines of code in very project, if you count in the concurrency safety...
But not everyone needs the concurrency safety. In fact, if one is building best practice synchronous APis then this concurrency is entirely unnecessary, as the Aggregate will be local to and only used by a single thread in a function, before being turned into the error interface, and returned to the caller.
Without the concurrency, there is 8 lines of code, as most of the expanded functionality you present (the Append and Len functions) are duplicates of existing basic functionality of the language.
And returning an empty &Aggregate{} will mean that the caller cannot just use if err != nil so before returning the Aggregate, I will need to have at the end of every Aggregate using function:
if aggregate.Count() > 0 {
return aggregate
}
return nil
Then there is the matter of https://github.com/golang/go/wiki/CodeReviewComments#declaring-empty-slices You should use &Aggregate{} and at which point calling a NewAggregate() function is actually more code in the caller than just &Aggregate{}.
I have been trying to delicately explain to you why—despite looking like a good idea—this code is not actually a good solution. Breaking out code and making it generic can actually be harmful to the code of callers and in the end increase the overall complexity of all of the code.
Not everything has to be broken out and made generic.
Just to be clear, I am bowing out of this discussion. I have said my piece, and there is nothing further my contribution could accomplish except further upsetting anyone.
Your previous comment made actually a very good point against my proposal.
And the links and examples where spot on and proved your point.
Thanks for your time and valuable feedback and sorry if you felt that way.