-
Notifications
You must be signed in to change notification settings - Fork 117
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(datastore): Update network connection availability checking status logic in ReachabilityMonitor of Datastore #2854
base: main
Are you sure you want to change the base?
Conversation
Hi @aladine, thank you for your contribution submission. We will prioritize reviewing this PR and reply back! |
Any update in this pull request? |
Hi @aladine, sorry for the delay. I will try and provide some feedback within next few days and suggest any updates if needed. |
No problem, I am happy to answer all the questions or concerns for this pull request. |
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.
Looks good to me! Only some minor stylistic things to clean up.
...datastore/src/main/java/com/amplifyframework/datastore/syncengine/NetworkCapabilitiesUtil.kt
Outdated
Show resolved
Hide resolved
aws-datastore/src/main/java/com/amplifyframework/datastore/syncengine/ReachabilityMonitor.kt
Outdated
Show resolved
Hide resolved
...store/src/test/java/com/amplifyframework/datastore/syncengine/NetworkCapabilitiesUtilTest.kt
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.
Overall changes look good. Can you take a look at an alternate implementation here: https://github.com/aws-amplify/amplify-android/tree/tjroach/improve-network-detection-datastore
I attempted to take in Matt's suggestions and simplify it a bit. I believe the code functions identically to yours. The tests would just need updated to use MockK
aws-datastore/src/main/java/com/amplifyframework/datastore/syncengine/ReachabilityMonitor.kt
Outdated
Show resolved
Hide resolved
...datastore/src/main/java/com/amplifyframework/datastore/syncengine/NetworkCapabilitiesUtil.kt
Outdated
Show resolved
Hide resolved
…ork/datastore/syncengine/NetworkCapabilitiesUtil.kt Co-authored-by: Matt Creaser <[email protected]>
Thanks Tyler, I move the codes from NetworkUtil to extensions folder as your branch. Update test location and mock method also. |
aws-datastore/src/main/java/com/amplifyframework/datastore/syncengine/ReachabilityMonitor.kt
Show resolved
Hide resolved
Hi all, a kind ping for the progress of this pull request. I can simplify my logic if it introduces churns to the current implementation. |
Issue #, if available:
#2738
Description of changes:
Enhance the capability to detect network connection availabilities in syncengine of aws datastore. This includes:
How did you test these changes?
(Please add a line here how the changes were tested)
Add unit test to verify the new change
Documentation update required?
General Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.