koin-annotations icon indicating copy to clipboard operation
koin-annotations copied to clipboard

Respect Default Arguments When Using `@Property`

Open undermark5 opened this issue 2 years ago • 15 comments

Is your feature request related to a problem? Please describe. I can define a component that takes an argument annotated with @Property and provide a default value, however, when the module is generated, the default value is completely ignored and only getProperty(<key>) is used rather than getProperty(<key>, <default>).

Describe the solution you'd like Ideally, during generation, the default value is acknowledged and respected in the result as described above.

Describe alternatives you've considered Alternative is to instead make the argument nullable, then using ?: where I'm using the property to get the default, but this sort of masks the fact there is a default property.

Target Koin project koin-annotations

undermark5 avatar Feb 19 '23 19:02 undermark5

problem is to deal with default arguments case. How to know if you want the default argument value when you use @Property 🤔

arnaudgiuliani avatar May 30 '23 13:05 arnaudgiuliani

Isn't the point of the default to be a fallback in case a bean definition isn't otherwise provided? How do they work without annotations? The specific use case I'm looking for is for a library to be able to define a default implementation of something but have the default bound looser than any that get defined by the library's user (similar to spring's ConditionalOnMissingBean behavior)

I did do some quick research and it appears to not be possible to get the default value of the parameter during KSP, but we could instead use an annotation value to define a default provider class that is used to generate a default if no other bean is defined.

undermark5 avatar May 30 '23 15:05 undermark5

I did do some quick research and it appears to not be possible to get the default value of the parameter during KSP, but we could instead use an annotation value to define a default provider class that is used to generate a default if no other bean is defined.

🤔 begins to be tricky. The initial case is to use Koin property if there is no default value right?

arnaudgiuliani avatar Aug 04 '23 09:08 arnaudgiuliani

Maybe this is overly complex and it also may not work out like I'm thinking, but something like this seems like it might work.

interface DefaultGenerator<T> {
    fun generate(): T
}

annotation class Property(/*...*/ defaultGenerator: KClass<out DefaultGenerator<*>>? = null)

You would then generate code that uses getProperty(<key>, <call the generate function if it exists>)

It's may not be worth the added complexity and at this point it seems easier to define the default in the code rather than the parameters, but I think the theory behind the above would work (there would obviously be some requirements that the implementation of the DefaultGenerator either has a no args constructor or is an object, and that the type of the parameter is assignable from the type of the type parameter from the implementation).

Ultimately, I think this is similar to how the @PreviewParameter annotation works in Compose.

undermark5 avatar Aug 04 '23 14:08 undermark5

:thinking: begins to be tricky. The initial case is to use Koin property if there is no default value right?

Default to anything provided by koin, falling back only if koin doesn't have a definition.

undermark5 avatar Aug 04 '23 14:08 undermark5

There may be some expanded/interesting use cases for this in relation to #54.

undermark5 avatar Aug 04 '23 14:08 undermark5

yes, seems to be around the same problem

arnaudgiuliani avatar Aug 04 '23 14:08 arnaudgiuliani

I'm stuck on some dev about accessing the default value expression. I'm waiting some support on this point 👍

arnaudgiuliani avatar Sep 06 '23 15:09 arnaudgiuliani

I'm stuck on some dev about accessing the default value expression. I'm waiting some support on this point 👍

I'm not sure I understand what you're saying? Are you waiting for some external support from KSP? Or are you saying you want some support on this in the form of a PR/contribution to the project?

undermark5 avatar Sep 06 '23 15:09 undermark5

Either design an annotation that provides the default value itself, or for now we are on hold. The solution would need us to go through IR exploration.

arnaudgiuliani avatar Sep 12 '23 08:09 arnaudgiuliani

https://github.com/InsertKoinIO/koin-annotations/issues/73#issuecomment-1665681467

The above is a solution. The annotation doesn't necessarily have to be separate from the existing @Property one and could be added as an argument to it (that defaults to null). Also, because the properties file is pretty much only able to provide strings, the above solution is rather overkill, and the default value could simply be added as a literal value to the annotation like @Property("key", "defaultValue") generated code then uses the getProperty("key", "defaultValue")

I sometimes keep forgetting this issue was specifically about the property annotation, and not arbitrary koin managed objects (which already have ways to provide defaults that others can override via module overloading). I believe that via the annotations, without this approach, there isn't a way of providing a default value for a property that happens to not be defined in a properties file.

If you really need me to, I can submit a PR, but I think the above should be enough to get an implementation.

undermark5 avatar Sep 12 '23 13:09 undermark5

@Property("key", "defaultValue") generated code then uses the getProperty("key", "defaultValue") yes, this was one of the initial states. I can go for this/

arnaudgiuliani avatar Oct 31 '23 07:10 arnaudgiuliani

The problem here is that we can't go with generic type value annotation class Property(val value: String, val defaultValue : Any) We need a String or another kind of compatible/primitive data.

The question is then, how to provide properties and default values with annotations way. 🤔

A proposal: Like tagging an existing property to say that's a default value for such property id:

@DefaultProperty("id")
val myDefaultValue : String = "_DEFAULT_"

@Single
class MyClass(@Property("id") val myValue : String)

this would generate

single { MyClass(getProperty("id",myDefaultValue)) }

arnaudgiuliani avatar Dec 06 '23 08:12 arnaudgiuliani

That's an idea, but what happens if multiple annotated values are provided for the same property? I guess we detect that and throw an error?

I'm also having Junit's @ValuSource annotation come to mind. It has multiple fields/properties but you only set one, the one that you set must correspond to the type of your parameter. Something similar could maybe be done here, though the tricky part is figuring out how to tell the difference between a set value and an unset value because annotations can't have nullable properties. Junit makes use of arrays, but I'm not certain if that makes sense here.

In short, the proposed approach looks good.

undermark5 avatar Dec 06 '23 13:12 undermark5

ok thanks 👍

arnaudgiuliani avatar Jan 30 '24 16:01 arnaudgiuliani

#141 is bringing @ProperyValue to add on a field, to associate the right Property default value

arnaudgiuliani avatar Jul 15 '24 15:07 arnaudgiuliani