Initial Kotlin Extensions for Toothpick #324

chenthorne · 2019-02-12T17:15:35Z

Adds two new modules, toothpick-kotlin and toothpick-kotlin-androidx, to provide kotlin friendly interfaces to common Toothpick use cases. Also provides some syntactic sugar for using Toothpick with Android Kotlin.

Both modules contain README's with examples on how to use the extensions and the smoothie-sample project has been updated to have a mirror of the existing samples but in kotlin.

See issues #305 and #316 or the draft extensions at https://github.com/Leafly-com/toothpick-extensions-sample

Things remaining:

Get community and project maintainer buy-in
Add tests for both modules
Determine if kotlin samples should be split off into their own module
Make sure CI is happy and checking

chenthorne · 2019-02-12T17:19:08Z

@code-twister with modules split

coveralls · 2019-02-13T23:00:06Z

Coverage decreased (-3.4%) to 92.323% when pulling f91a39f on Leafly-com:chenthore/kotlin-extensions into 54476ca on stephanenicolas:master.

stephanenicolas · 2019-03-30T02:28:45Z

smoothie-sample/src/main/java/com/example/smoothie/kotlin/LessSimpleActivity.kt

+        scope.installModules(module { String::class named "SpecialString" providedBy "My Special String" })
+        super.onCreate(savedInstanceState)
+        setContentView(R.layout.simple_activity)
+        ButterKnife.bind(this)


Do we still need butterknife ? It seems to me we're using a simple lazy to assign the views.

Not really, was just copying the Java examples over for parity.

stephanenicolas · 2019-03-30T03:00:25Z

toothpick-kotlin/src/main/java/toothpick/kotlin/ToothpickInjectDelegate.kt

+        if (injectConfigBlock != null) {
+            injectConfig.apply(injectConfigBlock)
+        }
+        val scope = injectConfig.scope ?: scope ?: getScope(thisRef, property) ?: throw IllegalStateException("No valid scope available when attempting to inject $property in $thisRef")


Let's finish with a . and explain how to fix the issue. If there is more than one alternative, let's explain them all.

stephanenicolas · 2019-04-01T02:01:20Z

toothpick-kotlin/src/main/java/toothpick/kotlin/ToothpickInjectDelegate.kt

+
+        if (thisRef is HasScope) {
+            return thisRef.scope
+        }


I got an issue with this. No idea if it is a standard way to implement other DI engines usually.
But it seems very hacky for a few reasons. I understand that we have this mechanism, and the fallback of creating a new scope right under the app for activities, because we want to be more concise and not pass the scope when doing by inject(scope)

But first things first, I am not sure I understand how lazy and inject differ now. Isn't the delegated property going to be "computed" only when the code accesses it ? if so, what's the difference with a lazy injection, and if not, when is it gonna be computed?

I don't really like the interface, and the fallback neither. It assumes that the activity as a given property, implements a given interface. An activity could decide to call it's scope scopeA and forget about the interface and no injection would take place.
Same for the fallback, we don't want to consider the activity's scope is placed right under the application by default. And for fragments, I am not even sure that the activity scope should be the default.

I was thinking that it would be maybe cleaner:

to pass the scope all the time when injecting / lazying a property. by inject(scope). We could also prefer to use a scope name by inject(this)

the activity can itself have a getter/delegate to create it's scope when needed

lateinit var scope: Scope get() { TP.createScope(...) scope.installModules(...) return scope }

if it's not convenient to pass the scope to every inject call. Can't we have another mechanism where we would restor the call to TP.inject(this, scope) and this would provide the scope to by inject() 's delegate?

And there is another issue. IMHO. We're loosing the @Inject annotation for fields. It seems to me there is a tension in your PR between what happened to butterknife (replaced by lateinit by findViewByID) and the classic @Inject annnotation. I prefer the annotation, but why did you prefer the other way, for speed ?

I'm not a fan of this hack. I haven't found a way to do this better without passing scope with every call. At one point, I had scope as nullable, so if you didn't provide it the delegate would try to use this. I didn't really like that either but it may be better than this route.

In most cases, I don't want to have to pass scope ever unless I'm doing something fancy.

I like no longer having the @Inject annotations, less non-code in my code to read around. We gain a few things with it:

No longer need an annotation processor, quicker build time

No longer need member injectors, quicker build time, no java leaking into pure Kotlin modules causing more compilation

Private injected members, no need to weaken access modifier to allow an APT generated class access.

No longer need to set as lateinit var or nullable

IMO, reads cleaner

Puts all injection logic in one spot as you can put scope and name in there as well, it's all done in one go instead of multiple annotations or split of logic.

we still need the factories to be created.

factories could be implemented in kotlin using extension function but I don't know any AP doing this. Not sure the build times would be any better.

that one ok

I am really motivated by keystroke count.@Inject lateinit var foo vs lateinit var foo by inject()
5...

It also changes the logic of the instantiation and scopes. Now the scope would be decided by the client whereas in java it's set by the entity itself. I feel that it might be conflicting in some cases too.

Yup, fair point for some classes, others won't need it like activities and fragments (nice for Android land, not applicable for Java land).

Sure, factories are still needed for most, and there is still Java leakage, but less and less to get rid of if TP ever moves to a kotlin based solution.

A big plus in my eyes as it influences 4

Actual code:
@Inject lateinit var foo: Foo
private val foo: Foo by inject()

I really like the second vs the first, and in the day of IDEs and code complete, keystroke count alone is a poor argument loosening access restrictions and adding mutability to the variable. Big philosophy of Kotlin is immutability, and this change moves TP in that direction.

In my use cases, it hasn't been much of a big deal as I rarely am doing anything special with scopes. And when I am, I'm already paying extra attention to how I'm using the scope. I'm not sure I follow what you mean by client and entity, could you elaborate?

Do kotlin people really talk about java leakage ? I find it funny ;)

I get it for #4. That's a good point (sorry I am really new to kotlin, it takes some time to put the pieces together ;)). Yes for immutability, it's a big win. But I like my so much my little cute annotations ! Snifff...

I understand why the PR goes a little too far with scopes now. ;)

stephanenicolas · 2019-04-01T02:03:36Z

toothpick-kotlin/src/main/java/toothpick/kotlin/ToothpickExtensions.kt

+    Toothpick.inject(toInject, this)
+}
+
+fun <T : Any> Scope.getInstance(clazz: KClass<T>, name: String? = null, annotationName: KClass<out Annotation>? = null): T {


do we need the non reified ones ?

When you want to inject a subclass into a super.

Is there an example in your PR ?

divide into two overloads to disambiguate name or annotationName, not both

stephanenicolas · 2019-04-01T02:07:04Z

toothpick-kotlin/src/main/java/toothpick/kotlin/ModuleDSL.kt

+        return module.bind(T::class.java) named name
+    }
+
+    inline infix fun <reified T : Any> KClass<T>.providedAs(clazz: KClass<out T>): Binding<T>.BoundStateForClassBinding {


all the fluent API has been renamed entirely. Can we keep the exact same method names for the bindings ?
I am not opposed to renaming them if needed, but it's a big change for TP. Are there any kotlin "way of doing things" that are at stake with these new names ?
Oh.. to is a kotlin keyword... right ?

I think the DSL should look more like:

bind T::class.java toInstance t0 bind T::class.java toClass T1::class.java bind T::class.java withName "foo" toInstance t0 bind T::class.java toProviderInstance p0 bind T::class.java toProviderClass P::class.java

for providesSingletonInScope, instancesInScope, singletonInScope, I agree we have an issue that a infix method needs a param...

Yeah, I went back and forth on this one a lot. Poked around in kodein and other kotlin DSLs to find a common naming scheme and didn't really see anything definitive. I settled on what I did as it read the most naturally to me as a human, but I'm not hugely attached to it. I like the examples you've provided.

I also like the with style ... but really the main problem is that to is a reserved word.. I don't find it great I must say.
Same goes for as , otherwise we could have had construct like

bind T::class.java to instance t0 bind T::class.java to instance T0::class.java as singleton

This would be the more natural solution.
Would it work if we would do

bind T::class.java `to` instance t0 bind T::class.java `to` instance T0::class.java `as` singleton

It's ugly, but I am trying to see all our options.

We could actually allow the to operator if module would support adding pairs or something that converts to a pair (destructuring)

module { bind Class<T> to instance(t0) bind Class<T> to Class<T1> bind Class<T> to providerInstance(p0) bind Class<T> to Class<Provider<P1>> }

we could maybe even use the as cast opertator:

bind Class<T> to Class<T1> as Singleton

For this to work, we would need to introduce a hierarchy like:
Singleton: BindingPair
where BindingPair would be convertable to a Pair.

We definitely need to have a clearer idea of what our current fluent API do / can do / should do. It's not clear yet and it's part of the problem we're having.

Interesting. I'll have to play with this to see if how it feels in real code. We're using a variant of the DSL internally so I can see how it goes with real world modules.

toothpick-kotlin/README.md

stephanenicolas · 2019-04-01T02:23:26Z

toothpick-kotlin/README.md

+// Lazy inject with scope search
+class ScopeSearch : HasScope {
+    override lateinit var scope: Scope
+        protected set


I would prefer an open get() that subclasses can override than a set.

I'm not sure I follow the comment. I settled with this as it provided universal/public access to the getter but only protected access to the setter. This keeps any ole class changing the scope.

stephanenicolas · 2019-04-01T02:25:23Z

One side effect of this PR is that member injectors are not needed for kotlin code.

stephanenicolas · 2019-04-01T03:19:29Z

@chenthorne can you have a look at kodeint DSL for bindings / module. They use the method with elegantly : https://github.com/Kodein-Framework/Kodein-DI

chenthorne · 2019-04-01T03:52:45Z

Thanks for the review!

@stephanenicolas Sure thing, I'll look through that again, anything in Kodein you'd like me to look at in particular? Did you look at Koin? Some of the stuff I did was driven by things I liked in there.

Looking forward to chatting with you about this soon.

stephanenicolas · 2019-04-01T05:39:32Z

Yeah I like the binding DSL in kodein. bind Foo::class.java with singleton { lambda }

…

On Sun., Mar. 31, 2019, 20:52 Cody Henthorne, ***@***.***> wrote: Thanks for the review! @stephanenicolas <https://github.com/stephanenicolas> Sure thing, I'll look through that again, anything in Kodein you'd like me to look at in particular? Did you look at Koin? Some of the stuff I did was driven by things I liked in there. Looking forward to chatting with you about this soon. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#324 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABv33WLHzudZgTOpc53cXuqrHUDAZYL8ks5vcYKNgaJpZM4a3a5S> .

stephanenicolas · 2019-04-01T16:28:04Z

smoothie-sample/build.gradle

@@ -70,3 +78,9 @@ dependencies {
  androidTestImplementation deps.easymock
  androidTestImplementation deps.dexmaker
 }
+
+tasks.withType(org.jetbrains.kotlin.gradle.tasks.KotlinCompile).all {


we should use Task Configuration Avoidance.

dlemures · 2019-10-18T17:17:18Z

Closing. We already released KTP.

Thx :)

Initial Kotlin Extensions for Toothpick

2fd83ab

chenthorne requested a review from dlemures February 12, 2019 17:17

Fix test failure

f91a39f

stephanenicolas reviewed Mar 30, 2019

View reviewed changes

stephanenicolas reviewed Apr 1, 2019

View reviewed changes

toothpick-kotlin/README.md Show resolved Hide resolved

stephanenicolas reviewed Apr 1, 2019

View reviewed changes

dlemures closed this Oct 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial Kotlin Extensions for Toothpick #324

Initial Kotlin Extensions for Toothpick #324

chenthorne commented Feb 12, 2019 •

edited

Loading

chenthorne commented Feb 12, 2019

coveralls commented Feb 13, 2019

stephanenicolas Mar 30, 2019

chenthorne Apr 1, 2019

stephanenicolas Mar 30, 2019

stephanenicolas Apr 1, 2019

stephanenicolas Apr 1, 2019

chenthorne Apr 1, 2019

stephanenicolas Apr 1, 2019

chenthorne Apr 1, 2019

stephanenicolas Apr 1, 2019

stephanenicolas Apr 1, 2019

chenthorne Apr 1, 2019

stephanenicolas Apr 1, 2019

chenthorne Apr 5, 2019

stephanenicolas Apr 1, 2019 •

edited

Loading

stephanenicolas Apr 1, 2019 •

edited

Loading

chenthorne Apr 1, 2019

stephanenicolas Apr 1, 2019 •

edited

Loading

stephanenicolas Apr 1, 2019 •

edited

Loading

chenthorne Apr 1, 2019

stephanenicolas Apr 1, 2019

chenthorne Apr 1, 2019

stephanenicolas commented Apr 1, 2019

stephanenicolas commented Apr 1, 2019

chenthorne commented Apr 1, 2019

stephanenicolas commented Apr 1, 2019 via email

stephanenicolas Apr 1, 2019

dlemures commented Oct 18, 2019

Initial Kotlin Extensions for Toothpick #324

Initial Kotlin Extensions for Toothpick #324

Conversation

chenthorne commented Feb 12, 2019 • edited Loading

chenthorne commented Feb 12, 2019

coveralls commented Feb 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephanenicolas Apr 1, 2019 • edited Loading

Choose a reason for hiding this comment

stephanenicolas Apr 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephanenicolas Apr 1, 2019 • edited Loading

Choose a reason for hiding this comment

stephanenicolas Apr 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephanenicolas commented Apr 1, 2019

stephanenicolas commented Apr 1, 2019

chenthorne commented Apr 1, 2019

stephanenicolas commented Apr 1, 2019 via email

Choose a reason for hiding this comment

dlemures commented Oct 18, 2019

chenthorne commented Feb 12, 2019 •

edited

Loading

stephanenicolas Apr 1, 2019 •

edited

Loading

stephanenicolas Apr 1, 2019 •

edited

Loading

stephanenicolas Apr 1, 2019 •

edited

Loading

stephanenicolas Apr 1, 2019 •

edited

Loading