Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add ApiFutures.catchingAsync #117

Merged
merged 2 commits into from Mar 13, 2020

Conversation

schmidt-sebastian
Copy link
Contributor

@schmidt-sebastian schmidt-sebastian commented Mar 11, 2020

I would like to use this in Firestore. I tried to match the existing signature that allows the callback to return '? extends V' but this does not seem to be possible (see transformAsync).

I would like to use this in Firestore. I tried to match the existing signature that allows the callback to return '? extends V' but this does not seem to be possible (see transformAsync).
@googlebot googlebot added the cla: yes label Mar 11, 2020
@codecov
Copy link

@codecov codecov bot commented Mar 11, 2020

Codecov Report

Merging #117 into master will increase coverage by 0.44%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #117      +/-   ##
============================================
+ Coverage     60.48%   60.93%   +0.44%     
- Complexity      147      148       +1     
============================================
  Files            14       14              
  Lines           615      622       +7     
  Branches         92       92              
============================================
+ Hits            372      379       +7     
  Misses          217      217              
  Partials         26       26              
Impacted Files Coverage Δ Complexity Δ
src/main/java/com/google/api/core/ApiFutures.java 85.71% <100.00%> (+2.38%) 11.00 <1.00> (+1.00)

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 f9320c3...9c12535. Read the comment docs.

Copy link
Contributor

@elharo elharo left a comment

Can we have a one-pager on what this is, what it's supposed to do, and why you need it? Without context it's hard to review constructively. Thanks.

@chingor13 chingor13 requested a review from vam-google Mar 11, 2020
@schmidt-sebastian
Copy link
Contributor Author

@schmidt-sebastian schmidt-sebastian commented Mar 11, 2020

This exposes "Futures.catchingAsync", similar to the existing "catching", which exposes "Futures.catching". The difference is that it allows an ApiAsyncFunction in its callback.

I am currently working on a PR that sits on top of this branch which can shed light on its usage.

@@ -104,6 +104,26 @@ public void onSuccess(V v) {
return new ListenableFutureToApiFuture<V>(catchingFuture);
}

public static <V, X extends Throwable> ApiFuture<V> catchingAsync(
Copy link
Collaborator

@vam-google vam-google Mar 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overal looks good to me, but can we mark this as @Beta API just in case?

Copy link
Contributor Author

@schmidt-sebastian schmidt-sebastian Mar 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@schmidt-sebastian
Copy link
Contributor Author

@schmidt-sebastian schmidt-sebastian commented Mar 11, 2020

@elharo Here is a PR that uses it googleapis/java-firestore@d6de6ce#diff-dc2367736d16b0b533729ca22cb69d50R169

catchingAsync allows me to centralize the error handling in one place.

@schmidt-sebastian
Copy link
Contributor Author

@schmidt-sebastian schmidt-sebastian commented Mar 13, 2020

@vam-google @elharo Is there anything else that needs to be done here?

Copy link
Collaborator

@vam-google vam-google left a comment

LGTM

@BenWhitehead BenWhitehead merged commit 8d40a11 into googleapis:master Mar 13, 2020
4 checks passed
@chingor13 chingor13 mentioned this pull request Mar 25, 2020
miraleung pushed a commit that referenced this issue Jun 4, 2020
Add new ApiFuture.catchingAsync method mirroring ApiFuture.catching, except allowing for an ApiAsyncFunction to be passed instead of ApiFunction.

I tried to match the existing signature that allows the callback to return '? extends V' but this does not seem to be possible (see transformAsync).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants