-
Notifications
You must be signed in to change notification settings - Fork 510
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
Fix #3310: Introduce Prod and Debug Implementation of NetworkConnectionUtil #3512
Conversation
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.
Good work @yashraj-01. Suggested nits majorly.
utility/src/main/java/org/oppia/android/util/networking/NetworkConnectionUtil.kt
Outdated
Show resolved
Hide resolved
utility/src/main/java/org/oppia/android/util/networking/NetworkConnectionUtilDebugImpl.kt
Show resolved
Hide resolved
utility/src/main/java/org/oppia/android/util/networking/NetworkConnectionUtilDebugImpl.kt
Outdated
Show resolved
Hide resolved
utility/src/main/java/org/oppia/android/util/networking/NetworkConnectionUtilDebugImpl.kt
Outdated
Show resolved
Hide resolved
utility/src/main/java/org/oppia/android/util/networking/NetworkConnectionUtilDebugImpl.kt
Outdated
Show resolved
Hide resolved
utility/src/main/java/org/oppia/android/util/networking/NetworkConnectionUtilProdImpl.kt
Outdated
Show resolved
Hide resolved
utility/src/main/java/org/oppia/android/util/networking/NetworkConnectionUtilProdImpl.kt
Show resolved
Hide resolved
utility/src/test/java/org/oppia/android/util/networking/NetworkConnectionUtilDebugImplTest.kt
Outdated
Show resolved
Hide resolved
Unassigning @Sarthak2601 since the review is done. |
Hi @yashraj-01, it looks like some changes were requested on this pull request by @Sarthak2601. PTAL. Thanks! |
Given my current load of PRs, I'll need to defer this until after @Sarthak2601 has approved. Could you please reassign this to me at that point @yashraj-01? |
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 have a nit suggestion for now. Please check the failing CI checks.
utility/src/main/java/org/oppia/android/util/networking/NetworkConnectionUtilProdModule.kt
Outdated
Show resolved
Hide resolved
…etworkconnectionutil
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.
Thanks @yashraj-01.Overall, I think this looks really good. Just had a few follow-up comments, otherwise the PR LGTM.
utility/src/main/java/org/oppia/android/util/networking/NetworkConnectionDebugUtil.kt
Outdated
Show resolved
Hide resolved
utility/src/main/java/org/oppia/android/util/networking/ConnectionStatus.kt
Show resolved
Hide resolved
utility/src/main/java/org/oppia/android/util/networking/NetworkConnectionDebugUtil.kt
Outdated
Show resolved
Hide resolved
utility/src/main/java/org/oppia/android/util/networking/NetworkConnectionUtilDebugImpl.kt
Outdated
Show resolved
Hide resolved
utility/src/main/java/org/oppia/android/util/networking/BUILD.bazel
Outdated
Show resolved
Hide resolved
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.
Thanks @yashraj-01. LGTM!
I'm inclined to merge this as-is since the previous test pass was fully green, but let's wait until all the tests complete. |
Unassigning @BenHenning since they have already approved the PR. |
Explanation
Fixes #3310. Introduced Prod and Debug implementations of NetworkConnectionUtil.
Screenshots
Robolectric tests passing for
ProdNetworkConnectionUtilTest
Robolectric tests passing for
DebugNetworkConnectionUtilTest
Robolectric tests passing for
NetworkConnectionTestUtilTest
Checklist