https://kotlinlang.org logo
Join Slack
Powered by
Hey <@U2JKKPMEE> - thank you for following up on t...
# koin-contributors
m
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
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
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
we can make a “poll” on slack/twitter and let people vote?
m
That sounds like a good idea. We probably can a lot of nice options from the community. 🙂
a
yeap 🙂 I will prepare something
m
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
the 
factoryOf
can be autocomplete in an easiest way, it come just after 
factory
but still thinking 🤔
m
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
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
Sorry, what do you mean with the qualifier overload problem? Because it is in the end instead of first parameter? 🤔
a
yes
it’s in the current API the first argument, and here we are making tricks around function signature
m
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
I tought that 2. was getting problems, while proposing 
viewModelOf
? 🤔
m
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
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
@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
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
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
can be interesting to have this as experimental, while launching a first shot of the API
let me think a bit again
m
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
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
That sounds awesome! 😄
👍 1
👀 ???
a
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
Oh, right! Changes done. 😄
a
Perfect 👌
m
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
I believe we just need a bit of documentation (docs folder) to describe the new DSL API
m
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.
5 Views
Open in Slack
PreviousNext
Powered by