Move binding coroutine implementation to gradle subproject

[Munzey]

After some discussion in #28, it makes most sense to move the binding coroutines implementation to a subproject and have it published as a separate artifact that users will need to pull as a dependency for this feature.

[michaelbull]

We should now be able to remove the test dependencies we had on kotlin coroutines from the root project, right?

We also need to think about the publishing code for this. I've never tried to configure publishing for both a root project & sub-project in the same repository. You can try the command gradlew publishToMavenLocal for it to publish under your ~/.m2/ local maven repository to see if it publishes properly.

[michaelbull]

We may want to consider making both the core implementation and the coroutines extension subprojects, removing the rootProject. It might make the publishing configuration easier and it's how I've traditionally done it in other projects. The okhttp repository is an example of this being somewhat of a standard approach.

e.g.

kotlin-result
  ├── example
  ├── kotlin-result
  └── kotlin-result-coroutines

[Munzey]

We may want to consider making both the core implementation and the coroutines extension subprojects, removing the rootProject

@michaelbull agreed from my tries at publishToMavenLocal this afternoon with the current structure it seems like it might be the easier route, plus like you say there seems to be lots of projects/stack overflows that tackle the issue with this approach.

Will try take a look at updating to that structure over the weekend.

[michaelbull]

Hi @Munzey, what's the status of this? Did you get a chance to look over the weekend regarding putting things in separate subprojects? Let me know if you want me to jump in on this, but happy to let you lead the change since you're the owner of this part of the codebase (the Binding/Coroutine stuff).

[Munzey]

hey @michaelbull sorry I was staycationing down the country for the last week 😎 🌦️ So haven't gotten a chance to try change the structure to both being sub projects. I should have a chance later this week to take a look if that's not too late.

On a separate note. kotlin 1.4 is here 🎉 Been trying to get through all the notes - looks like there might be a few changes again to be made to bump to 1.4: e.g. https://kotlinlang.org/docs/reference/whatsnew14.html#specifying-dependencies-only-once looks like they don't want coroutine core lib to be too specific anymore which I think is a good thing!

In fact they added a migration guide: https://kotlinlang.org/docs/reference/migrating-multiplatform-project-to-14.html

[michaelbull]

Thanks @Munzey, no worries and no rush. Looks like there are other interesting parts in 1.4 too, like the explicit API mode which we probably want to start using.

[Munzey]

@michaelbull ok looks like I got publishJvmPublicationToMavenLocal task working as expected with the new project structure. I did this with signing commented out so if you can check that is still working as expected that would be great.

[michaelbull]

The release plugin (used to tag, build, push) writes its version and infers the project's name from the gradle.properties file, so I don't think moving the fields to the core/build.gradle.kts will work for us

[Munzey]

@michaelbull I think thats everything 🥵
Due to the size and number of changes already present in this pr, what do you think of leaving the kotlin 1.4 migration to a separate pr?

[michaelbull]

Absolutely, we need to get the release out with the fix and deprecation before considering 1.4. Also it will give people more of an incentive to migrate away from the broken code if the fixed code is behind the 1.4 upgrade ;)

[michaelbull]

Just looking over the current state again. I think we're almost there. One thing I would like to change is to make the coroutines project look more like the core project in naming & coordinates.

Right now we have

• kotlin-result published as com.michael-bull.kotlin-result:kotlin-result:VERSION
• coroutines published as com.michael-bull.kotlin-result.coroutines:coroutines:VERSION

It should look like:

• kotlin-result published as com.michael-bull.kotlin-result:kotlin-result:VERSION
• kotlin-result-coroutines published as com.michael-bull.kotlin-result:kotlin-result-coroutines:VERSION

I reckon the change necessary is to rename the coroutines subproject to kotlin-result-coroutines and lift the group declaration out of the subprojects and back into the root gradle.properties file.

[Munzey]

@michaelbull sure I can take a look at that later.

Something else I thought of overnight - I think I’ve gone and made the change still breaking. To stop the suspendable binding definitions clashing I renamed the package the old deprecated version thats in kotlin-result to coroutineslegacy I wasn’t thinking of the fact that this still means users who update the version could have a compile failure due to incorrect/missing import (though i set the deprecated level to Error, so technically would have failed anyway - perhaps we want to set that back to just warning as well). I guess to keep backwards compatibility, ideally the package of the new coroutines lib would be different. Can you think of a name for the package?

[michaelbull]

Maybe we could put the binding behaviour specifically under: com.github.michaelbull.result.coroutines.binding?

Leaves us more room to put other packages under the coroutines directory too and not have binding pollute it.

[Munzey]

@michaelbull regarding

It should look like:

kotlin-result published as com.michael-bull.kotlin-result:kotlin-result:VERSION
kotlin-result-coroutines published as com.michael-bull.kotlin-result:kotlin-result-coroutines:VERSION

This should now be fixed 😄 .

And regarding my comment

I guess to keep backwards compatibility, ideally the package of the new coroutines lib would be different.

I changed the name of the legacy binding package back to what it was, and added an extra package dir to the new kotlin-result-coroutines package so that the new SuspendableBinding.kt now lives in package com.github.michaelbull.result.coroutines.binding. I think this makes a lot of sense actually as it leaves the door open for other potential packages in the coroutines project, not just a solution for binding.

[Munzey]

Maybe we could put the binding behaviour specifically under: com.github.michaelbull.result.coroutines.binding? Leaves us more room to put other packages under the coroutines directory too and not have binding pollute it.

😮 woah we must have wrote this same time. I guess we were on the same wavelength lol

[Munzey]

Finally, thought id clean up the pom's description of the coroutine lib, and add a blurb to the README

[michaelbull]

Hi @Munzey. Squashed and tidied it up into a single commit and got it into master:

https://github.com/michaelbull/kotlin-result/commit/b16fb559a14dcd77139fb907db213184581e6bd6

Can you have a once over and make sure everything works, like the benchmarks etc? I've confirmed signing works locally (there are a lot more artifacts to sign now.. even more button presses on my Yubikey!)

If you're happy with it I will go ahead and work on updating the dependencies to Kotlin 1.4 and endeavour to get a release out on the weekend.

[Munzey]

Can you have a once over and make sure everything works, like the benchmarks etc?

Checked it out there. All tests passing and benchmarks running as expected! Looks like we're good to go 🚀

[michaelbull]

@Munzey

https://github.com/michaelbull/kotlin-result/blob/master/kotlin-result-coroutines/build.gradle.kts#L19

Are these dependencies correct? common dependending on the native impl of kotlin coroutines?

[Munzey]

@michaelbull yes that is correct, if i recall kotlinx-coroutines-core is only for jvm. I believe that kotlinx-coroutines-core-common will also work there. In 1.4 this is no longer supported/required and you only need to call kotlinx-coroutines-core. Maybe double check though now im doubting myself haha

[michaelbull]

Okay, I've already updated master to 1.4 so maybe you could have a go at seeing how many dependencies we can remove without breaking it? :)

[Munzey]

sure thing, I am free to do that tomorrow and I'll open a pr 👍