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

Add option to define coroutineContext for coroutine functions #387

Merged
merged 3 commits into from Jul 22, 2018

Conversation

markGilchrist
Copy link
Collaborator

this will prevent networkonmainthread exception

@codecov
Copy link

codecov bot commented Jul 15, 2018

Codecov Report

Merging #387 into master will decrease coverage by 0.19%.
The diff coverage is 40%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master     #387     +/-   ##
===========================================
- Coverage     76.45%   76.25%   -0.2%     
  Complexity      193      193             
===========================================
  Files            33       33             
  Lines           930      935      +5     
  Branches        161      164      +3     
===========================================
+ Hits            711      713      +2     
  Misses          134      134             
- Partials         85       88      +3
Impacted Files Coverage Δ Complexity Δ
...com/github/kittinunf/fuel/coroutines/Coroutines.kt 40.74% <40%> (-0.17%) 0 <0> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 52849fe...9f893d4. Read the comment docs.

@avaitla
Copy link

avaitla commented Jul 15, 2018

Was the CommonPool context in the coroutines code there before and accidentally removed or is there some other reason it needs to be here for the caller to only invoke launch(UI)? Not too familiar with coroutines, and am curious to know why it worked before.

@kittinunf
Copy link
Owner

I think this is reasonable to merge. Do you think we should add test for checking on the execution thread as well? So that we could have tests that cover us in the future.

@markGilchrist
Copy link
Collaborator Author

@kittinunf I have had a look at writing a test for the execution thread and I have run into a bit of a dead end. if you perform a test inside runBlocking the that forces the scope to be run on the blocking thread.

If anyone has any wise ideas they would be very gratefully received

@markGilchrist markGilchrist changed the title texts this via the sample app,ATP Add option to define coroutineContext for coroutine functions Jul 21, 2018
@kittinunf
Copy link
Owner

I haven't had any good idea either ... let's merge it.

@markGilchrist markGilchrist merged commit c314b0f into master Jul 22, 2018
@markGilchrist markGilchrist deleted the resolve_issue_386_adding_commonpool_scope branch July 22, 2018 07:20
@kittinunf kittinunf mentioned this pull request Aug 1, 2018
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.

None yet

3 participants