Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add class named & string/class qualifier #108

Merged

Conversation

ghasemdev
Copy link
Contributor

No description provided.

@MaxBQb
Copy link

MaxBQb commented Dec 24, 2023

Can you also support complex types?

@ghasemdev
Copy link
Contributor Author

Can you also support complex types?

Why do you need such a complex structure?

@MaxBQb
Copy link

MaxBQb commented Dec 25, 2023

Well, imagine Mapper<MyDomain, MyView>
we inject it using @Factory and we also need to add type qualifier, and just MyDomain alone isn't enough.
Or Imagine we use Serializer<List<MyDomain>>, so we need to add type qualifier not for List, not for MyDomain, but for List<MyDomain>, so here tree-structure is preferable for such usecases (and not flat one, because we must distinguish SomeClass<Int, OtherClass<Int>> and SomeClass<Int, OtherClass, Int>)
Such tree structure required only at annotation definition, and then can be converted to string for example, to be easily compared (see code I linked above).

So that's why I think that if we adding support for type-based qualifiers, then we need to at least support generic-types.

@ghasemdev
Copy link
Contributor Author

ghasemdev commented Dec 25, 2023

Well, imagine Mapper<MyDomain, MyView> we inject it using @Factory and we also need to add type qualifier, and just MyDomain alone isn't enough. Or Imagine we use Serializer<List<MyDomain>>, so we need to add type qualifier not for List, not for MyDomain, but for List<MyDomain>, so here tree-structure is preferable for such usecases (and not flat one, because we must distinguish SomeClass<Int, OtherClass<Int>> and SomeClass<Int, OtherClass, Int>) Such tree structure required only at annotation definition, and then can be converted to string for example, to be easily compared (see code I linked above).

So that's why I think that if we adding support for type-based qualifiers, then we need to at least support generic-types.

possible solution

Nested Structure

sealed interface ListQualifier {
  data object Int : ListQualifier
  sealed interface Str : ListQualifier {
    data object String : Str
  }
}

@Module
class ListModule {
  @Singleton(createdAtStart = true)
  @Qualifier(ListQualifier.Int::class)
  fun provideIntList() = listOf(1, 2, 3)

  @Singleton(createdAtStart = true)
  @Qualifier(ListQualifier.Str.String::class)
  fun provideStringList() = listOf("hi", "hello")
}

class MyActivity : ActivityScope() {
  private val intList: List<Int> by inject(qualifier<ListQualifier.Int>())
  private val stringList: List<String> by inject(qualifier<ListQualifier.Str.String>())
}

Flat Structure

annotation class IntListQualifier
annotation class StringListQualifier

@Module
class ListModule {
  @Singleton(createdAtStart = true)
  @Qualifier(IntListQualifier::class)
  fun provideIntList() = listOf(1, 2, 3)

  @Singleton(createdAtStart = true)
  @Qualifier(StringListQualifier::class)
  fun provideStringList() = listOf("hi", "hello")
}

class MyActivity : ActivityScope() {
  private val intList: List<Int> by inject(qualifier<IntListQualifier>())
  private val stringList: List<String> by inject(qualifier<StringListQualifier>())
}

Generated Code

Nested Structure

single(qualifier=org.koin.core.qualifier.StringQualifier("com.parsuomash.sdk.di.qualifier.ListQualifier\$Int"),createdAtStart=true) { moduleInstance.provideIntList() } bind(kotlin.collections.List::class)
single(qualifier=org.koin.core.qualifier.StringQualifier("com.parsuomash.sdk.di.qualifier.ListQualifier\$Str\$String"),createdAtStart=true) { moduleInstance.provideStringList() } bind(kotlin.collections.List::class)

Flat Structure

single(qualifier=org.koin.core.qualifier.StringQualifier("com.parsuomash.sdk.di.qualifier.IntListQualifier"),createdAtStart=true) { moduleInstance.provideIntList() } bind(kotlin.collections.List::class)
single(qualifier=org.koin.core.qualifier.StringQualifier("com.parsuomash.sdk.di.qualifier.StringListQualifier"),createdAtStart=true) { moduleInstance.provideStringList() } bind(kotlin.collections.List::class)

@MaxBQb
Copy link

MaxBQb commented Dec 25, 2023

Well that looks kinda hacky. The flat structure looks interesting (except it quickly gets verbose).
But it does not create clear connection between real kotlin types and qualifier, so it will be hard to query instances later on.
I mean, while simple querying is still trivial, it will be quite hard to retrieve Mapper<Domain, View> for specific Domain and View, known at compile-time. You'll end up building yet another big when condition to get MapperDomain1View1, MapperDomain2View2 ... or Mapper.Domain1.View1, Mapper.Domain2.View2.
With my solution, on the other hand you'll still be able to use FlatMeaninglessAnnotation for trivial usecases, but you will also able to (with the help of an inline function) retrieve instances for class with specific generics. So I can write one little function getMapper which will build TypeQualifier and quickly find what I need.

inline function (aka typeQualifierOf) should be part of API (I'll try to write/suggest one, later on).

@MaxBQb
Copy link

MaxBQb commented Dec 26, 2023

Here is an example of how typeQualifierOf may looks like.

import kotlin.reflect.KClass

annotation class TypeQualifier(val value: KClass<*>, vararg val params: TypeQualifier)

val TypeQualifier.qualifier: String get() {
    var qualifier = value.qualifiedName!!
    if (params.isNotEmpty())
        qualifier += "<" + params.joinToString { it.qualifier } + ">"
    return qualifier
}

inline fun <reified T : Any> typeQualifierOf(value: T): TypeQualifier = when (value) {
    is KClass<*> -> TypeQualifier(value)
    is TypeQualifier -> value
    else -> TypeQualifier(T::class)
}

inline fun typeQualifierOf(value: Any, vararg params: Any)
    = TypeQualifier(typeQualifierOf(value).value, *params.map { typeQualifierOf(it) }.toTypedArray())

inline fun <reified T : Any> typeQualifierOfT() = typeQualifierOf(T::class)
inline fun <reified T : Any, reified T1 : Any> typeQualifierOfT2() = typeQualifierOf(T::class, T1::class)
inline fun <reified T : Any, reified T1 : Any, reified T2 : Any> typeQualifierOfT3() = typeQualifierOf(T::class, T1::class, T2::class)
inline fun <reified T : Any, reified T1 : Any, reified T2 : Any, reified T3 : Any> typeQualifierOfT4() = typeQualifierOf(T::class, T1::class, T2::class, T3::class)

operator fun TypeQualifier.plus(other: KClass<*>)
    = this + other.toTypeQualifier()

operator fun KClass<*>.plus(other: TypeQualifier)
    = toTypeQualifier() + other
 
fun TypeQualifier.addParam(other: TypeQualifier)
    = TypeQualifier(this.value, *params, other)

fun KClass<*>.toTypeQualifier() = typeQualifierOf(this)

// Usage example
inline fun typeQualifierOfMapOf(KType: Any, VType: Any)
    = typeQualifierOf(Map::class, KType, VType)

// K, V can't be of complex type, but it's shorter
inline fun <reified K : Any, reified V : Any> typeQualifierOfMapOf()
    = Map::class + typeQualifierOfT2<K, V>()

fun main() {
    println(typeQualifierOfMapOf(typeQualifierOfMapOf<Int, String>(), String::class).qualifier)
}

Use of inline functions with reified types is discussional, since it allows only simple types to be used.

Also.. here is a DSL alternative

import kotlin.reflect.KClass

annotation class TypeQualifier(val value: KClass<*>, vararg val params: TypeQualifier)

val TypeQualifier.qualifier: String get() {
    var qualifier = value.qualifiedName!!
    if (params.isNotEmpty())
        qualifier += "<" + params.joinToString { it.qualifier } + ">"
    return qualifier
}

inline fun <reified T : Any> typeQualifierOf(value: T): TypeQualifier = when (value) {
    is KClass<*> -> TypeQualifier(value)
    is TypeQualifier -> value
    else -> TypeQualifier(T::class)
}

inline fun typeQualifierOf(value: Any, vararg params: Any)
    = TypeQualifier(typeQualifierOf(value).value, *params.map { typeQualifierOf(it) }.toTypedArray())

operator fun TypeQualifier.plus(other: TypeQualifier)
    = TypeQualifier(this.value, *params, TypeQualifier(other.value), *other.params)

fun TypeQualifier.addParam(other: TypeQualifier)
    = TypeQualifier(this.value, *params, other)

operator fun TypeQualifier.plus(other: KClass<*>)
    = this + other.toTypeQualifier()
operator fun KClass<*>.plus(other: TypeQualifier)
    = toTypeQualifier() + other

fun KClass<*>.toTypeQualifier() = typeQualifierOf(this)

interface TypeQualifierBuilder {
    val qualifier: TypeQualifier
    operator fun KClass<*>.unaryPlus()
    operator fun KClass<*>.invoke(builder: TypeQualifierBuilder.() -> Unit)
}

inline fun <reified T> TypeQualifierBuilder.add() { +T::class }

private class TypeQualifierBuilderImpl : TypeQualifierBuilder {
    private var _qualifier: TypeQualifier? = null
    override val qualifier get() = _qualifier ?: Unit::class.toTypeQualifier()

    private fun addTypeQualifier(value: TypeQualifier) {
        _qualifier = _qualifier?.addParam(value) ?: value
    }

    override fun KClass<*>.unaryPlus() {
        addTypeQualifier(this.toTypeQualifier())
    }
    override fun KClass<*>.invoke(builder: TypeQualifierBuilder.() -> Unit) {
        val newQualifier = TypeQualifierBuilderImpl().apply {
            +this@invoke
            builder()
        }.qualifier
        addTypeQualifier(newQualifier)
    }
}

fun buildTypeQualifier(builder: TypeQualifierBuilder.() -> Unit)
    = TypeQualifierBuilderImpl().apply(builder).qualifier

fun main() {
    val typeQualifier = buildTypeQualifier {
        Map::class {
            Map::class {
                add<Int>()
                +String::class
            }
            add<String>()
        }
    }
    println(typeQualifier.qualifier)
}

@MaxBQb
Copy link

MaxBQb commented Dec 26, 2023

Also please provide any feedback on my reasoning and code above, because, well, I see reflection of our discussion in commit name, but not in code changes itself.

@ghasemdev
Copy link
Contributor Author

Also please provide any feedback on my reasoning and code above, because, well, I see reflection of our discussion in commit name, but not in code changes itself.

I think this method can also be implemented in another PR.

@arnaudgiuliani

@MaxBQb
Copy link

MaxBQb commented Dec 26, 2023

Anyway your current annotation can't be used to build complex type structure (yeah that recursive-annotation thing, what I suggest, looks even more horrible than my DSL), am I right?

@MaxBQb
Copy link

MaxBQb commented Dec 27, 2023

I should clarify my intentions: even if u don't want to implement support for complex types by yourself, please at least add support for that at annotation side, so next changes can be implemented incrementally, without need of breaking existing interface (without need of rewrite TypeQualifier annotation)

@MaxBQb
Copy link

MaxBQb commented Dec 27, 2023

Ghm.. this whole time we can just use typeOf... My bad, I've almost forgot about such simple thing.

import kotlin.reflect.KClass
import kotlin.reflect.KType
import kotlin.reflect.jvm.jvmErasure
import kotlin.reflect.typeOf

annotation class TypeQualifier(val value: KClass<*>, vararg val params: TypeQualifier)

val TypeQualifier.qualifier: String get() {
    var qualifier = value.qualifiedName!!
    if (params.isNotEmpty())
        qualifier += "<" + params.joinToString { it.qualifier } + ">"
    return qualifier
}

fun typeQualifierOf(type: KType): TypeQualifier
    = TypeQualifier(type.jvmErasure, *type.arguments.map {
        typeQualifierOf(it.type!!)
    }.toTypedArray())
    
inline fun <reified T : Any> typeQualifierOf() = typeQualifierOf(typeOf<T>())

fun main() {
    println(typeQualifierOf<Map<Map<Int, String>, String>>().qualifier)
}

@MaxBQb
Copy link

MaxBQb commented Dec 27, 2023

So.. now such simple thing can be implemented in the scope of the same PR, what do you think?

@Fando
Copy link

Fando commented Jan 10, 2024

Would be valuable if defining type annotations could be less verbose. Usually a qualifying annotation is used in many places in the code causing it to look cluttered.

Instead of this:

@Scope(name = "scope1")  

@Named(name = "name1") 

Prefer less verbose definitions like:

annotation class Scope1

annotation class Name1

This way @Scope1 could be used instead of @Scope(name = "scope1")

Signed-off-by: Ghasem Shirdel <ghasem79.dev@gmail.com>
Copy link
Member

@arnaudgiuliani arnaudgiuliani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job

@arnaudgiuliani
Copy link
Member

same about preparing 1.4.0 branch - #107

@Neyasbit
Copy link

Neyasbit commented Apr 2, 2024

I tried to do that. And it only works

@Named("CoroutineDispatcherIO")
annotation class NamedCoroutineDispatcherIO

@Named("CoroutineDispatcherMain")
annotation class NamedCoroutineDispatcherMain
@Single
@NamedCoroutineDispatcherIO
fun provideDispatcherIO(): CoroutineDispatcher = Dispatchers.IO

@Single
@NamedCoroutineDispatcherMain
fun provideDispatcherMain(): CoroutineDispatcher = Dispatchers.Main

But there is a bug. When using two annotations at the same time. It turns out that there will be an instance of one object. Although it should be different.

class Test(
    @NamedCoroutineDispatcherIO dispatcherIO: CoroutineDispatcher,
    @NamedCoroutineDispatcherMain dispatcherMain: CoroutineDispatcher
) {
    init {
        // Wrong
        println(dispatcherIO.hashCode() == dispatcherMain.hashCode()) // True (expect false)
    }
}

@extmkv
Copy link

extmkv commented May 4, 2024

@ghasemdev when are you planning to merge it?

Signed-off-by: Ghasem Shirdel <ghasem79.dev@gmail.com>
@ghasemdev ghasemdev force-pushed the feat-class-named-qualifier branch from 8939e47 to 29e9b4e Compare May 4, 2024 19:25
@arnaudgiuliani arnaudgiuliani changed the base branch from main to 1.4.0 June 27, 2024 14:55
@arnaudgiuliani arnaudgiuliani merged commit e14b5e9 into InsertKoinIO:1.4.0 Jun 27, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants