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

Unify usage of coroutine scope across project #234

Merged
merged 6 commits into from
May 3, 2024

Conversation

yzueger
Copy link
Collaborator

@yzueger yzueger commented May 1, 2024

Closes #208
The refactors the coroutine scope usage so that basically everything is callable from a viewmodel coroutine scope. Only in the actual model implementations of repositories that directly talk to a database/webserver or something we switch to the IO sope using withContext(Dispatchers.IO) {}

@KlaKalma KlaKalma added this to the M2 milestone May 1, 2024
Copy link
Collaborator

@IB-12 IB-12 left a comment

Choose a reason for hiding this comment

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

Sonarcloud doesn't like us setting the dispatchers ourself, but I'm not sure there really is a way around that. LGTM, make sure to add the needed coverage and I'll approve it again 👍

Edit: Hadn't seen Tsogt's post

@yzueger yzueger force-pushed the bugfix/unify-coroutine-scopes branch from 2600f7c to e7d5589 Compare May 2, 2024 13:42
Copy link
Collaborator

@aymanbakiri aymanbakiri left a comment

Choose a reason for hiding this comment

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

forget to approve

Copy link
Collaborator

@aymanbakiri aymanbakiri left a comment

Choose a reason for hiding this comment

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

Thanks for updating the coroutine scope usage in our project. Now, everything can be called from a viewModel coroutine scope. You have also corrected the issue of manually setting the dispatchers by injecting dispatchers instead. I think you only need to increase the coverage and it will be ready to merge. Thanks for your good work.

Copy link

sonarqubecloud bot commented May 3, 2024

Copy link
Collaborator

@KlaKalma KlaKalma left a comment

Choose a reason for hiding this comment

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

Seems perfect, thanks for that :)

@yzueger yzueger dismissed tsogtb’s stale review May 3, 2024 10:32

Implemented changes

@yzueger yzueger merged commit 2146ce3 into main May 3, 2024
2 checks passed
@yzueger yzueger deleted the bugfix/unify-coroutine-scopes branch May 3, 2024 10:32
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.

Use same coroutine scope everywhere
5 participants