spotify / completable-futures

Utilities for working with futures in Java 8
Apache License 2.0
393 stars 51 forks source link

Add CompletableFutures.successfulAsList #40

Closed johanburatti closed 8 years ago

johanburatti commented 8 years ago

When migrating from guava ListenableFuture, we missed the Futures.successfulAsList. This implements the same functionality for CompletableFutures.

codecov-io commented 8 years ago

Current coverage is 100%

Merging #40 into master will not change coverage

@@            master       #40   diff @@
========================================
  Files            4         4          
  Lines           61        54     -7   
  Methods          0         0          
  Messages         0         0          
  Branches         3         3          
========================================
- Hits            61        54     -7   
  Misses           0         0          
  Partials         0         0          

Powered by Codecov. Last updated by 61233d8...d180917

mbruggmann commented 8 years ago

👍

mbruggmann commented 8 years ago

Thinking about it some more, I might actually prefer to not encourage successfulAsList in Java8 and rather do something like:

Stream.of(a, b)
  .map(this::someAsyncFunction)
  .map(fallbackTo(null))
  .collect(joinList())

This makes it more obvious at the call site that you introduce null values in the result. Any strong opinions @dflemstr @spkrka?

spkrka commented 8 years ago

I haven't though about it a whole lot, but my gut feeling agrees with you. Alternatively we could have successfulAsList(defaultValue, futures) or successfulAsList(t -> toValue(t), futures) - then it would also be more obvious how failures are converted to values instead of always returning null

johanburatti commented 8 years ago

I think the points you make are very valid. When migrating from ListanebleFuture we only duplicated that behavior, which I agree is not optimal.

Personally I think I like the suggestion of @spkrka with a lambda since that is still much more convenient to type and easier to understand when reading than the stream-based one. @mbruggmann if you agree, I will change successfulAsList to require a function from throwable to value as well.

johanburatti commented 8 years ago

Updated after comments

mbruggmann commented 8 years ago

👍 for the implementation. Only nitpick left: Should this really be successfulAsList now or rather an overload of allAsList?

johanburatti commented 8 years ago

Regarding naming it allAsListnow, I disagree. I think it is different enough to be named something else. Also, successfulAsList is what the corresponding guava method is called.

johanburatti commented 8 years ago

Sorry about the delay... Updated PR with rebased change. Added javadoc for .

mbruggmann commented 8 years ago

I think this is good to merge now. @dflemstr @spkrka any last minute concerns?

spkrka commented 8 years ago

No strong opinions from me