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

#246, #168: Add Kotlin Coroutines support #313

Merged
merged 6 commits into from
Apr 29, 2018
Merged

#246, #168: Add Kotlin Coroutines support #313

merged 6 commits into from
Apr 29, 2018

Conversation

lucasvsme
Copy link

@lucasvsme lucasvsme commented Feb 23, 2018

This pull requests adds some suspend functions to the Request class. They were added in a new module called fuel-coroutines and are intended to allows the developers to avoid callbacks while using asynchronous calls.

* Add suspend extension functions to Response objects
* Add integration tests to them
@codecov
Copy link

codecov bot commented Feb 23, 2018

Codecov Report

Merging #313 into master will increase coverage by 0.18%.
The diff coverage is 61.11%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #313      +/-   ##
============================================
+ Coverage     77.21%   77.39%   +0.18%     
+ Complexity      193      190       -3     
============================================
  Files            32       32              
  Lines           904      898       -6     
  Branches        148      152       +4     
============================================
- Hits            698      695       -3     
+ Misses          131      122       -9     
- Partials         75       81       +6
Impacted Files Coverage Δ Complexity Δ
...com/github/kittinunf/fuel/coroutines/Coroutines.kt 61.11% <61.11%> (ø) 0 <0> (?)
.../kotlin/com/github/kittinunf/fuel/core/Encoding.kt 89.58% <0%> (-2.73%) 13% <0%> (-2%)
...tlin/com/github/kittinunf/fuel/core/FuelManager.kt 88.76% <0%> (-0.93%) 31% <0%> (ø)
...tlin/com/github/kittinunf/fuel/util/FuelRouting.kt 100% <0%> (ø) 0% <0%> (ø) ⬇️
.../java/com/github/kittinunf/fuel/forge/FuelForge.kt
...f/fuel/core/interceptors/RedirectionInterceptor.kt 68.75% <0%> (+6.25%) 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 1a1891c...f5281f6. Read the comment docs.

@yoavst
Copy link
Collaborator

yoavst commented Feb 23, 2018

Do we want to allow the user to select in which coroutine pool it will make the network request?
Currently it uses Executors.newCachedThreadPool.

@lucasvsme
Copy link
Author

Do you mean wrapping the suspend function with async { } or launch { }? Something like:

fun main(args: Array<String>) = runBlocking {
    val call = async(CommonPool) { "https://httpbin.org/ip".httpGet().awaitStringResult() }
    val ipInJson = call.await()
    println(ipInJson)
}

@iNoles
Copy link
Collaborator

iNoles commented Feb 23, 2018

Android has own coroutine context for UI too.

@raharrison
Copy link

Doesn't Fuel already run in it's own ExecutorService? We don't want to start the coroutine on another thread which in turn calls Fuel which passes the actual request onto another thread.

@lucasvsme
Copy link
Author

@iNoles I've added a Espresso test to covers the Android use case you've mention.

@raharrison The Request class uses a Future internally and also the ExecutorService you've mentioned. Since these fields are final and private I couldn't find a way to change them while implementing coroutines as a module apart from core.

@kittinunf
Copy link
Owner

@lucasvalenteds First of all, thanks for this!! It has been a pretty long awaited feature.

What do you think about relaxing them so that it can be modified?

@lucasvsme
Copy link
Author

@kittinunf Your welcome. Thank you for the Fuel project!

About your question, I can't see how to make this coroutines module more flexible. I'm also failing to understand what it means in terms of implementation. Could you provide a use case to clarify how the developer can modify the module to get a better usage experience?

@raharrison
Copy link

Can this get merged as it still presents a solution to using Fuel with coroutines, even if more enhancements are needed in the future?

@kittinunf
Copy link
Owner

@lucasvalenteds would you be so generous applying the finishing touch?

@lucasvsme
Copy link
Author

@kittinunf Sure. Tell me what you need.

@kittinunf
Copy link
Owner

I think if you can resolve conflict then this is good to go.

@kittinunf
Copy link
Owner

it looks like kotlinCoroutinesVersion is missing from the build.gradle file?

The version was removed from a bad merge.
@lucasvsme
Copy link
Author

lucasvsme commented Apr 28, 2018

You are right, @kittinunf. Sorry about that. I don't have the Android development environment right now, so I'll need a couple of hours to review the full code in case the CI fails again.

* Migrate `compile` to `implementation`
* Migrate `testCompile` to `testImplementation`
* Change timeout from 15000 to 30000
* Split problematic test into three small tests
* Improve assertion related to exception of `awaitString()` method
@kittinunf
Copy link
Owner

Sure no rush, I plan to release new version with this PR so as soon as it is ready, I will make it.

Thanks for this though. It has been long overdue. I think you deserve a lot of credit for your work here :)

@lucasvsme
Copy link
Author

The CI has failing due to tests related to fuel module, which was not changed in this pull request. Do you have any idea why? I've run the fuel-coroutines tests and they are passing.

screenshot from 2018-04-28 06-36-59

@kittinunf
Copy link
Owner

Let's merge this in. I will investigate from master.

@kittinunf kittinunf merged commit 5f1d550 into kittinunf:master Apr 29, 2018
@lucasvsme lucasvsme deleted the coroutines-jvm branch May 5, 2018 23:06
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.

5 participants