Closed danielfosseliusspot closed 6 years ago
Merging #59 into master will not change coverage. The diff coverage is
100%
.
@@ Coverage Diff @@
## master #59 +/- ##
=======================================
Coverage 100% 100%
- Complexity 69 73 +4
=======================================
Files 6 7 +1
Lines 191 208 +17
Branches 9 9
=======================================
+ Hits 191 208 +17
Impacted Files | Coverage Δ | Complexity Δ | |
---|---|---|---|
...n/java/com/spotify/futures/CompletableFutures.java | 100% <100%> (ø) |
43 <2> (+2) |
:arrow_up: |
src/main/java/com/spotify/futures/Function7.java | 100% <100%> (ø) |
2 <2> (?) |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 06af851...18fee84. Read the comment docs.
@mattnworb, @mbruggmann PTAL
I don't feel very qualified to review this, but I've added a few other reviewers.
The change looks good, but I am wondering if it would be better to implement something similar to Futures.join
in https://github.com/spotify/futures-extra#joining-multiple-futures instead - that would work for any number of futures to join in a typesafe way.
I have no problems with checking on different solutions, so you mean that in the method combineFutures i would use FuturesExtra.join() instead? The case i have is to extend this method: https://ghe.spotify.net/agglib/agglib-common/blob/master/agglib-taste/src/main/java/com/spotify/taste/clients/TasteClient.java#L85 with two more futures
i was able to solve this in another way
I have a case when i need seven futures to combine