https://kotlinlang.org logo
#koin-contributors
Title
# koin-contributors
m

Marcello Galhardo

11/04/2021, 3:19 PM
Hey @arnaud.giuliani - thank you for following up on that. I’m right now playing around with that to see how it goes (I can do a PR in the next days with my changes after I add comments, etc). In total, the new API would add 33 new methods (
single
,
factory
,
viewModel
) to reduce the
new
. I think that is OK because I don’t foresee many new binding definitions to be created in the future. That is how it would look like (see the picture for comparison):
Copy code
factory(::RecoveryReminderViewModel)
Locally is already working, there is only one points that still concerns me: the namespace / auto complete might be a little polluted (see picture below). What do you think? If you don’t see a problem I think we can move forward and I can do PRs with it next days. Maybe someone else has other opinions here too. 🙂
👍 1
Actually, the resolution is working very well… except by the
viewModel
(as both are extension functions). 🤔
Overload resolution ambiguity. All these functions match.
Maybe we can consider a “prefix” for those functions to resolve the VM one? E.g.,
autoFactory
,
autoViewModel
or something like that? (auto is not a good eaxmple, but I don’t know what would be an ideal word).
bindFactory
, bind would be nice but I guess is out of the table now that Koin has a bind API already. Maybe
resolveFactory
?
a

arnaud.giuliani

11/04/2021, 5:08 PM
yeah, this one important point. It can be sometime already hard to read some of the Koin API. The problem would be to pollute existing API 🤔
first, I’m still wondering if “10” constructor arguments is something to consider 😛 but hard to prevent it
perhaps, try to find a DSL wording very close
because even if we make an extension on
Module.factory()
, the IDE will autocomplete with all available APIs
resolve
implies something already “technical” for me ... let’s find some good ideas
newFactory(::MyComponent)
or
factoryOf(::MyComponent)
m

Marcello Galhardo

11/04/2021, 5:15 PM
I kind of like factoryOf but that would still give us all options when typing
factory
as
factoryOf
might be a candidate for resolution. Additionally,
of
in Kotlin implies a list of somesort… 🤔
About the 10, I find it kind of reasonable. I used originally 22 due to Kotlin functional interfaces: https://github.com/JetBrains/kotlin/blob/master/spec-docs/function-types.md#functions-with-022-parameters-at-runtime
😁 1
I think for now,
newFactory
,
newViewModel
and
newSingle
are the best options. 🤔
a

arnaud.giuliani

11/04/2021, 5:17 PM
we can make a “poll” on slack/twitter and let people vote?
m

Marcello Galhardo

11/04/2021, 5:17 PM
That sounds like a good idea. We probably can a lot of nice options from the community. 🙂
a

arnaud.giuliani

11/04/2021, 5:17 PM
yeap 🙂 I will prepare something
m

Marcello Galhardo

11/04/2021, 5:17 PM
Thank you! 👍
👍 1
Just brainstorming:
make
might be a good option.
makeFactory
,
makeViewModel
and
makeSingle
. It doesn’t match any other Koin API, and it is not as common as
new
in the JVM ecosystem.
new
can also be renamed to
make
and keep same concept. We can call it “make API” to distinguish it. And it is also short. 🙂
a

arnaud.giuliani

11/04/2021, 5:57 PM
the
factoryOf
can be autocomplete in an easiest way, it come just after
factory
but still thinking 🤔
m

Marcello Galhardo

11/05/2021, 12:36 AM
Ok, right now we have the following ideas: • newFactory(::Repository) • makeFactory(::Repository) • factoryOf(::Repository) • factoryFor(::Repository) 🤔
👍 1
I’m playing around with the possibilities, it actually nicer with the suffix. The
viewModelOf
feels a little like it will return something but so far is the best option IMO - reads well, auto complete is very much good, feels natural with other Kotlin APIs (e.g.,
mutableStateOf
or
listOf
). I updated the PR using this approach for now, so feel free to clone and give it a try. 🙂
a

arnaud.giuliani

11/05/2021, 10:38 AM
Ok thanks 🙂 I will ask some community about that, I will prepare a message to ask their feelings. It will confirm it (or not 😛 )
👍 1
yeah people like
*Of
. But I’m struggling with the qualifier overload problem 😕
👍 1
m

Marcello Galhardo

11/12/2021, 6:02 PM
Sorry, what do you mean with the qualifier overload problem? Because it is in the end instead of first parameter? 🤔
a

arnaud.giuliani

11/15/2021, 9:59 AM
yes
it’s in the current API the first argument, and here we are making tricks around function signature
m

Marcello Galhardo

11/15/2021, 3:07 PM
I only see 3 options here: 1. We go with the
new
method and completely drop the current Pull Request as it is more composable as a side effect of having some extra ceremony (my initial proposal). 2. We keep the two overloads for each method, as it allow us to maintain consistency with our previous API. For our API consumers, that removes boilerplate and they don’t need to know about the manual overloading (I just need to revert my previous commit). 3. We keep the current order and breaking consistency. That is a little worse in API perspective but there is some advantages: Kotlin won’t suggest an anonymous lambda like it does if the constructor is in the end and you can still use named parameters to keep the order you prefer. All those options have their advantage and disadvantages - from the current work we already did and the fact we are trying to reduce ceremony, option 2 sounds like the best one. We put the ceremony in us in favor to provide a better API for our users - also, I don’t expect those methods to change for a long-long time...
a

arnaud.giuliani

11/16/2021, 7:05 AM
I tought that 2. was getting problems, while proposing
viewModelOf
? 🤔
m

Marcello Galhardo

11/16/2021, 9:22 AM
Hm, let me rephrase point 2. It is the following solution:
Copy code
fun viewModelOf(constructor)

fun viewModelOf(qualifier, constructor)
Manually writing the two signatures (with and without the qualifier) works fine. I changed it only because of your comment in the PR:
concerning qualifier overload signature, I was thinking perhaps to have the constructor first and then nullable qualifier.
If that option looks good I can change my PR to include it. 🤔
a

arnaud.giuliani

11/16/2021, 9:39 AM
ok, then we could have clearly
viewModelOf(::constructor)
and
viewModelOf(qualifier, ::constructor)
there is not signature conflict then? (sorry, just to be sure 😛 )
If that option looks good I can change my PR to include it. 🤔
yes
just thinking in what version we can make it land
m

Marcello Galhardo

11/16/2021, 1:03 PM
@arnaud.giuliani I reverted my last commit to keep the changes as option 2. To confirm: there is no signature conflict - we are good to go. 😄 Well, if we follow SemVer it would probably be
3.2.0
. We are adding functionality in a backward compatible manner. Patches are only for bug-fixes which isn’t the case here. The new features are really nice but it isn't big enough for a major. If we have some luck, we can include the
Module Grouping
in version
3.2.0
, otherwise it would need to go to
3.3.0
- it is your call. 🤔 I'm not sure how you normally announce releases but I think those features could become a short Twitter Thread or Blog Post - as they will reduce tons of boilerplate. (:
👍 1
a

arnaud.giuliani

11/16/2021, 4:38 PM
yes, this is why I was looking to open a first 3.2.0 branch, keeping those new features on a new 3.2.0 branch yes
the
new
operator is in 3.1.x … I can check how to move it 🤔
For the announce, there is the koin developer hub, where I (should) write regularly about incoming stuff
and you’re welcome to talk about it also on your side
👍 1
m

Marcello Galhardo

11/16/2021, 10:22 PM
Hm, as there is no breaking change in the current
3.1.x
- isn’t it easier to simple to bump it and start a new release cycle for
3.2.x
and so on? We can patch 3.1 inside 3.2 as they are backward compatible... 🤔 If for some reason that is not possible, we don’t necessary need to change a lot - we could add an
experimental
contract annotation to the
new
method so developers should not use it - and not communicate it officially right now as an in test feature - while only the
3.2.x
release will have the annotation removed.
a

arnaud.giuliani

11/17/2021, 7:32 PM
can be interesting to have this as experimental, while launching a first shot of the API
let me think a bit again
m

Marcello Galhardo

11/17/2021, 8:31 PM
What I meant is: for the 3.2.0 I think it is fine to have the new API released normally as it is very straightforward. We can mark it as experimental only on 3.1.x - so we don't need to revert it or remove from there as the changes are merged in the 3.1.x. That way we can do the way you said above. What do you think?
a

arnaud.giuliani

11/18/2021, 1:04 PM
I will clean up branches, to avoid bringing unstable stuff on main 3.1.x branch. I will prepare a 3.2.x branch.
👍 1
With that, it can be possible to publish intermediate alpha/beta versions to have some feedback
m

Marcello Galhardo

11/18/2021, 2:27 PM
That sounds awesome! 😄
👍 1
👀 ???
a

arnaud.giuliani

11/19/2021, 10:08 AM
what ?!
I’ve changed master branch to
main
I’m reorganising dev support branches to open 3.2.x ... but then it closed
master
sorry 🙏
m

Marcello Galhardo

11/19/2021, 1:00 PM
Oh, right! Changes done. 😄
a

arnaud.giuliani

11/19/2021, 1:19 PM
Perfect 👌
m

Marcello Galhardo

12/08/2021, 10:13 AM
Hey @arnaud.giuliani, not sure if I understand (asking here to have a fast feedback loop):
I can help to setup some doc around
a

arnaud.giuliani

12/09/2021, 9:11 AM
I believe we just need a bit of documentation (docs folder) to describe the new DSL API
m

Marcello Galhardo

12/09/2021, 12:32 PM
I see - I will be traveling during next days so I will be offline for few weeks. Feel free to move forward without me - otherwise when I get back I can try to collaborate with the docs.
2 Views