michaelbull / kotlin-result

A multiplatform Result monad for modelling success or failure operations.
ISC License
1.05k stars 63 forks source link

convert to mpp structure #18

Closed Munzey closed 4 years ago

Munzey commented 4 years ago

Proposed solution for #12 Things that had to change:

michaelbull commented 4 years ago

Hi @Munzey, don't want to leave you in silence but the PR looks good right now. I'm busy right now so will hopefully get some time this weekend to merge it and release. I tried last weekend to release the binding behaviour but I had connection issues with uploading artifacts to maven central. Apologies for trailing on this one.

Munzey commented 4 years ago

@michaelbull No worries! Appreciate the update :relaxed:

michaelbull commented 4 years ago

Hi @Munzey. Been thinking about this change a bit today. Given we introduced binding before MPP but now the binding is platform dependent (expects a platform-specific impl of that Exception object we spoke about), I guess that means that we would need to add an equivalent implementation for every platform we support - whereas before the binding stuff we could have technically supported every platform as none of the code was platform specific in any way. I may be misunderstanding this though?

I was also therefore thinking of splitting out the binding stuff into a separate subproject and therefore separate artifact, such that the main library is platform independent and the binding stuff is platform dependent. I'm not sure how I feel about this and it could get ugly pretty quickly, maybe I'm misunderstanding the situation though. Let me know your thoughts.

Munzey commented 4 years ago

whereas before the binding stuff we could have technically supported every platform as none of the code was platform specific in any way. I may be misunderstanding this though?

@michaelbull I think you are right yes.

I was also therefore thinking of splitting out the binding stuff into a separate subproject and therefore separate artifact, such that the main library is platform independent and the binding stuff is platform dependent. I'm not sure how I feel about this and it could get ugly pretty quickly,

This does sound like it would become a pain to support both. Some questions around it too, would it be two dependencies to add to a project like mpp.result-core and jvm.result-binding? or is there a mpp.result-core and a jvm.result-extended that comprises everything? Looking at a project like arrow, I think it can become confusing when there is multiple artifacts required to pull all with names that seem to suggest they do something similar 😅 .

Its a shame we can't have an expected BindException with a default implementation in the common code. There is actually an open issue to add this to kotlin (which I will now be up voting haha): https://youtrack.jetbrains.com/issue/KT-20427?_ga=2.19951672.1617774961.1592496293-2122254457.1527159951

The alternative here is to just not override the stacktrace for the jvm, I decided to look for some articles to mention how much faster is it to not fill in the stack trace. I did find one: https://www.atlassian.com/blog/archives/if_you_use_exceptions_for_path_control_dont_fill_in_the_stac that suggests it is quite beneficial when used as part of flow control as it is here. Not sure if the current benchmarking we have written could be used to give much of an indicator.

Going back to the original open issue, I actually wonder is there even added value right now for making the project mpp to support android. The only thing preventing me pulling this lib into an android project was to specify that I wanted kotlin to compile against jvm 1.8. Thats all it took. So It doesn't really need to be an mpp project to support android as far as I understand (I would assume for a kotlin native project that builds for both iOS and android that you could specify deps that are only for the android side of code).

That then leaves mpp projects where kotlin code is to be cross compiled to run for that platform , which right now I can imagine would be JS or Swift. Which comes back to your point

I guess that means that we would need to add an equivalent implementation for every platform we support

I think its really up to you whether you think its worth it 😄 Personally I'm very much in favor of keeping binding as part of the main lib, and simply adding the cross platform support for languages as the issues are opened. To hedge our bets I would say if we added a swift (or maybe it has to be obj-c?) actual and a js actual and added those to the gradle mpp builds, there probably wouldnt be many requests coming in.

TL;DR im not sure lol

avently commented 4 years ago

Everyday I open this page to look at the status of merging process. That's because I really need functionality that this library gives but I can't use it without multiplatform support. So i'm just wondering, will this pr be merged in near future or it can be unmerged for months?

Munzey commented 4 years ago

@avently can I ask what platforms you plan to support with your project?

avently commented 4 years ago

@Munzey Android and Java right now, Linux/Windows/Mac/iOS later.

michaelbull commented 4 years ago

Merged in bca344daafadbc7af42089b88c712ac1061298dd

avently commented 4 years ago

@Munzey and @michaelbull thank you, guys!