RedApparat / Fotoapparat

Making Camera for Android more friendly. 📸
Apache License 2.0
3.81k stars 405 forks source link

Update to Kotlin 1.3 and migrate Coroutines away from the experimental package #302

Closed jossiwolf closed 5 years ago

jossiwolf commented 5 years ago

Kotlin 1.3 is getting closer and Coroutines finally are graduating to stable 🎉 With that, all Coroutines classes are moved out of the experimental package which breaks Fotoapparat when using the Kotlin 1.3 RC.

This PR updates Kotlin to 1.3 RC 146 and migrates to stable Coroutines. I suggest merging this onto a separate branch and publishing a version for devs using Kotlin 1.3 RC until Kotlin 1.3 Stable has been released.

Feedback appreciated!

Diolor commented 5 years ago

ouch didn't think this migration part. Makes sense. The even more correct way would be to actually have "implementation" dependencies instead of "compile" but I couldn't make jitpack work with them back then. Maybe there is there some room for improvement.

Tests are failing btw

jossiwolf commented 5 years ago

ouch didn't think this migration part. Makes sense. The even more correct way would be to actually have "implementation" dependencies instead of "compile" but I couldn't make jitpack work with them back then. Maybe there is there some room for improvement.

I can take a look at that later on.

Tests are failing btw

Yep, looking at why that is right now. FileLoggerTest#Write Message is failing for me on master too.

jossiwolf commented 5 years ago

So after speaking with a JetBrains employee, @Diolor and I found out that there's an issue with Mockito as with the graduation of Coroutines to stable, the Continuation class was of course moved out of the experimental package which resulted in issues while mocking. The corresponding issue can be found here: https://github.com/mockito/mockito/pull/1501

5d2ae96 fixes the failing tests by using the Mockito fork of @dzharkov.

jossiwolf commented 5 years ago

The PR by @dzharkov was merged into Mockito and Mockito 2.23.0 containing the patch has been released. With 42d9859 the official Mockito version is used again and everything works!

@Diolor This PR should now be ready to be merged onto a separate branch :)

jossiwolf commented 5 years ago

@Diolor It'd be awesome if you could merge this onto a separate branch now and create a new Maven release :)

dmitry-zaitsev commented 5 years ago

I would prefer to stick to stable Kotlin release. Since we removed jitpack integration branch deployment will not work as easily, so you can instead deploy it on jitpack using your fork. That should be quite easy.

Nevertheless, thanks for the submission! Let's merge it right after Kotlin 1.3 is out.

fleficher commented 5 years ago

Hi 👋 Just wanted to let you know (if it's not already done 😉 ) that Coroutines 1.0.0-RC1 is out with kotlin 1.3.0-rc-190. Final version is coming really soon (i hope !).

The kotlin-eap repository is now removable 🎆

Diolor commented 5 years ago

Hey guys, I just merged #315 which was ready to be merged that this one so I will close this one. Though thanks tons @jossiwolf for dealing with the mockito issue 🎉