Add AppendInvoke convenience wrapper #47
Conversation
|
|
Codecov Report
@@ Coverage Diff @@
## master #47 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 1 1
Lines 102 105 +3
=========================================
+ Hits 102 105 +3
Continue to review full report at Codecov.
|
prskr
force-pushed
the
46-append-call
branch
3 times, most recently
from
97f8485
to
42dee48
Compare
prskr
changed the title
prskr
changed the title
This takes a pass over the provided documentation to add more examples and explanations.
This adds more information about AppendInvoke to the package level documentation.
This simplifies the tests for multierr.AppendInvoke with multierr.Close. Primarily, this moves the shared logic of building a mock closer and using multierr.Close into the test body rather than in individual test cases.
|
Thanks for the contribution, @baez90! This is generally looking pretty good. CC @prashantv |
|
Thanks for your work on the documentation! |
|
@prashantv any chance to get a review any time soon? I'd love to be able to use this 😄 |
| } | ||
|
|
||
| func TestAppendInvokeClose(t *testing.T) { | ||
| tests := []struct { |
There was a problem hiding this comment.
looks like these cases are exactly the same as AppendInvoke, should we instead share the cases? Then we could have the same cases be used across all variances:
- an
Invokefunction that returns the specified error - an object implementing
Invokerthat returns the specified error Closer- possible also with
AppendInvokeFnif we add that method
There was a problem hiding this comment.
Interesting idea, unfortunately this is not directly possible.
You would need to recreate the inputs for every case that would 'reuse' the input because the into modifies the original input and I'm not aware of any good solution how to handle this without introducing either weird side effects or rather complex test input generation.
| defer func() { | ||
| fmt.Println(err) | ||
| }() |
There was a problem hiding this comment.
optional: I wonder if we should do a full file example instead, as it may be easier to see the print from a separate function that calls a function using AppendInvoke. It's a little easier to think of this as the result the caller sees IMO
There was a problem hiding this comment.
you mean moving this to a new file only containing this very example and refactoring the anonymous function to normal functions?
Sorry for the delay in reviewing, I started reviewing a few weeks ago but never finished my review, thanks for the reminder! |
|
I applied as much of the comments as I though that would make sense - can this be merged now? |
We never documented AppendInto at the package level. This adds a basic explanation of the function.
|
Apologies for the delay @baez90. It's been a bit busy recently. |
Follow up to #47 with the following changes. - The tests for AppendInvoke with Close duplicate the tests for AppendInvoke. These can be merged, and the unit tests for Close only need to verify whether the Invoker returned by multierr.Close calls the Close function in the provided io.Closer. - Replace AppendInvoke example with something more realistic. - Update changelog.
|
Released in v1.7.0. Thanks for the contribution and your patience, @baez90! |
This is meant as a WIP PR for issues #46
For now this shall only showcase the proposed change and allow further discussions on different aspects of the proposed change to ease the overall process.