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

Async task/async task to coroutines #3236

Conversation

farhazulMullick
Copy link

@farhazulMullick farhazulMullick commented May 15, 2021

Related Issues

App:
AsyncTask_to_Coroutines #2714

Library PR (if needed):

  • Added changelog files for the fixed issues in folder changelog/unreleased. More info here
    -. Added Coroutines Support to all AsyncTask class

QA

@CLAassistant
Copy link

CLAassistant commented May 15, 2021

CLA assistant check
All committers have signed the CLA.

@farhazulMullick
Copy link
Author

@ChrisEdS
would you like to review my pull request

@abelgardep
Copy link
Contributor

Hi @farhazulMullick, this is not exactly what we expect. We are replacing AsyncTasks with coroutines but in a different way. We are moving to MVVM architecture and this logic may belong to a ViewModel or another module, so we are replacing them step by step. BTW we are adding unit tests to assure they work the way they are supposed.

That's why I closed #2714

Anyway, thanks for your interest.

@abelgardep abelgardep closed this May 17, 2021
@hannesa2
Copy link
Contributor

We are replacing ... is there an open pull request or a branch without pull request ?

You can anyway accept this pull request and continue your hidden replacing

It's not smart to close an un-merged pull request. This causes mostly to see this as last pull request from this person and you loose one additional of your few contributors.

@abelgardep
Copy link
Contributor

Maybe "We are replacing" is not the best expression.

I meant that we have already replaced some async tasks during the new architecture process. For example AuthenticatorAsyncTask was replaced in #2826 and that logic was moved to domain and data module with their corresponding unit tests.

That's why I closed the issue. We are doing this along the way 👍

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.

4 participants