Add AppendInto function #31
Conversation
This adds an AppendInto function that behaves similarly to Append
except, it operates on a `*error` on the left side and it reports
whether the right side error was non-nil.
func AppendInto(*error, error) (errored bool)
Making the left side a pointer aligns with the fast path of `Append`.
Returning whether the right error was non-nil aligns with the standard
`if err := ...; err != nil` pattern.
```diff
-if err := thing(); err != nil {
+if multierr.AppendInto(&err, thing()) {
continue
}
```
Resolves #21
Codecov Report
@@ Coverage Diff @@
## master #31 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 2 2
Lines 116 123 +7
=====================================
+ Hits 116 123 +7
Continue to review full report at Codecov.
|
| // for line := range lines { | ||
| // var item Item | ||
| // if multierr.AppendInto(&err, parse(line, &item)) { | ||
| // continue |
There was a problem hiding this comment.
nit: I'd write this in the negative, ala "if-not-append"
There was a problem hiding this comment.
I'd like to keep this 1:1 comparable to the non-AppendInto version for which err != nil would be more standard.
I can add more code to the success path if that would alleviate this concern.
error.go
Outdated
| // items = append(items, item) | ||
| // } | ||
| // | ||
| // Compare the loop body above with a version that relies solely on Append. |
There was a problem hiding this comment.
nit: sentences like this, and above circa L413, are usually "Compare with this:" or "Bla bla bla; for example:"
error.go
Outdated
| if err == nil { | ||
| return false | ||
| } | ||
| // We will panic if 'into' is nil. This is not documented above |
There was a problem hiding this comment.
Yes, but what's the failure look like? Would it still be better to add an explicit nil guard here, so that we can write a better panic message?
There was a problem hiding this comment.
Fair. How about a panic with the message "'into' must not be a nil pointer"?
|
The only thing really preventing my from accepting, is perhaps improving the debug-ability of that panic path for when it gets mis-used. |
This adds trailing colons to comments preceding examples.
This explicitly checks if into was nil and panics with a more informative error message than we would get with just `*into`.
prashantv
left a comment
prashantv
left a comment
There was a problem hiding this comment.
Nice, I like how small the change ended up being!
This adds an AppendInto function that behaves similarly to Append
except, it operates on a
*erroron the left side and it reportswhether the right side error was non-nil.
Making the left side a pointer aligns with the fast path of
Append.Returning whether the right error was non-nil aligns with the standard
if err := ...; err != nilpattern.Resolves #21