-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
refactor(database): migrate to paging 3 #1983
refactor(database): migrate to paging 3 #1983
Conversation
…aseUI-Android into rpf-rtdb-paging-3 � Conflicts: � database/src/main/java/com/firebase/ui/database/paging/FirebaseRecyclerPagingAdapter.java
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approve, just one comment you may want to address before I merge. Thank you for this!
} | ||
|
||
return Single.fromCallable(() -> { | ||
Tasks.await(task); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will throw if the Tasks fails:
https://developers.google.com/android/reference/com/google/android/gms/tasks/Tasks#await(com.google.android.gms.tasks.Task%3CTResult%3E)
So we can't rely on getting to the next line. Although I think it's fine since you re-throw at the bottom, but I just don't think you'll ever get that far.
Also we can probably remove the task.isSuccessful
nesting since we know if we even get to the next line, the task is successful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@samtstern Interesting, I didn't know that.
I have wrapped it around a try/catch so that developers using FirebaseUI can get the original cause instead of await
's ExecutionException
.
This PR should:
See #1982