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

Feature/bma/sdk sync service #6029

Merged
merged 6 commits into from
Jun 8, 2022
Merged

Feature/bma/sdk sync service #6029

merged 6 commits into from
Jun 8, 2022

Conversation

bmarty
Copy link
Member

@bmarty bmarty commented May 11, 2022

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

First point of #5864

Motivation and context

Improve SDK API

Screenshots / GIFs

Tests

  • In progress

Tested devices

  • Physical
  • Emulator
  • OS version(s):

Checklist

@github-actions
Copy link

github-actions bot commented May 11, 2022

Unit Test Results

142 files  +18  142 suites  +18   2m 18s ⏱️ +9s
229 tests +  9  229 ✔️ +  9  0 💤 ±0  0 ±0 
760 runs  +34  760 ✔️ +34  0 💤 ±0  0 ±0 

Results for commit a33f7d0. ± Comparison against base commit f5b4e89.

♻️ This comment has been updated with latest results.

@bmarty bmarty force-pushed the feature/bma/sdk_sync_service branch from 4448255 to 6e86b02 Compare May 25, 2022 13:26
override fun startSync(fromForeground: Boolean) {
Timber.i("Starting sync thread")
// TODO How to check that now?
// assert(isOpen)
Copy link
Member Author

Choose a reason for hiding this comment

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

Is it OK to not do this check anymore? See the TODO.

Copy link
Contributor

Choose a reason for hiding this comment

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

For SyncService.doSync (SyncAndroidService now) this assert worked as exit condition. AssertionError was caught and handled and now it won't. Do we know how often it happens? And what will happen if we'll try to start sync without open session - will it fail before network request or will be executed? We shouldn't execute a request if there is no chance that it will result with success 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right. Improved in a33f7d0 which restores the previous checks.


override fun stopSync() {
// TODO How to check that now?
// assert(isOpen)
Copy link
Member Author

Choose a reason for hiding this comment

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

Is it OK to not do this check anymore? See the TODO.

Copy link
Member Author

Choose a reason for hiding this comment

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

@bmarty bmarty requested review from a team, Claire1817 and fedrunov and removed request for a team May 25, 2022 13:36
@bmarty bmarty marked this pull request as ready for review May 25, 2022 13:36
@bmarty
Copy link
Member Author

bmarty commented Jun 8, 2022

Thanks for the review. I will rebase to fix all the conflicts, and merge it.

@bmarty bmarty force-pushed the feature/bma/sdk_sync_service branch from a33f7d0 to ac61aee Compare June 8, 2022 08:04
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 8, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

26.9% 26.9% Coverage
0.0% 0.0% Duplication

@bmarty bmarty merged commit 3b2f9d9 into develop Jun 8, 2022
@bmarty bmarty deleted the feature/bma/sdk_sync_service branch June 8, 2022 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants