Add support for transformer function in config property delegate

[marschwar]

This relates to a comment by @BraisGabin in #3670 addressing situations in which the configured values must be transformed.

This PR:

• allows config values to be converted into anything else using a transformer function (by lambda or method reference)
• adds memoization to all config values (similar to LazyRegex)
• increases test coverage for ConfigurationCollector and ConfigProperty
• removes Long from supported config types as it is not supported by BaseConfig

Examples:

val aSet: Set<String> by config(listOf("a", "b")) { it.toSet() }

val aRegex: Regex by config("[0-9]*", String::toRegex)
    
val anotherRegex: Regex by configWithFallback("fallback", "[0-9]*") { it.toRegex() }

This would also resolve #3672

[codecov]

Codecov Report

Merging #3676 (c23a7c7) into main (e41de94) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #3676      +/-   ##
============================================
+ Coverage     78.75%   78.80%   +0.04%     
- Complexity     2900     2901       +1     
============================================
  Files           473      473              
  Lines          9336     9335       -1     
  Branches       1722     1723       +1     
============================================
+ Hits           7353     7356       +3     
+ Misses         1075     1073       -2     
+ Partials        908      906       -2     

Impacted Files	Coverage Δ	Complexity Δ
...b/arturbosch/detekt/api/internal/ConfigProperty.kt	100.00% <100.00%> (+5.55%)	0.00 <0.00> (ø)
...ekt/generator/collection/ConfigurationCollector.kt	77.10% <100.00%> (+1.82%)	29.00 <8.00> (+1.00)
...rturbosch/detekt/rules/complexity/ComplexMethod.kt	95.23% <100.00%> (-0.12%)	13.00 <0.00> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e41de94...c23a7c7. Read the comment docs.

[BraisGabin]

I think that we need a it more flexibility. Something like:

fun <T : Any> config(
    defaultValue: String,
    map: ((String) -> T)? = null
): ReadOnlyProperty<ConfigAware, T> = SimpleConfigProperty(defaultValue)

And we need to think about caching these values. toRegex() can be a costly call if we call it a lot of times.

[marschwar]

I think that should be possible. As long as we do not expand the types that are used for defaultValue we should be fine when it comes to inferring the default value.

[picklebento]

And we need to think about caching these values. toRegex() can be a costly call if we call it a lot of times.

+1

[BraisGabin]

What do you think about this:

fun config(
    defaultValue: List<String>
): ReadOnlyProperty<ConfigAware, List<String>> = ListConfigProperty(defaultValue) { it }

fun <T : Any> config(
    defaultValue: List<String>,
    transform: (List<String>) -> T
): ReadOnlyProperty<ConfigAware, T> = ListConfigProperty(defaultValue, transform)


fun <T : Any> config(
    defaultValue: T
): ReadOnlyProperty<ConfigAware, T> = TransformedConfigProperty(defaultValue) { it }

fun <T : Any, U : Any> config(
    defaultValue: T,
    transform: (T) -> U,
): ReadOnlyProperty<ConfigAware, U> = TransformedConfigProperty(defaultValue, transform)

private class TransformedConfigProperty<T : Any, U : Any>(
    private val defaultValue: T,
    private val transform: (T) -> U
) : ReadOnlyProperty<ConfigAware, U> {
    override fun getValue(thisRef: ConfigAware, property: KProperty<*>): U {
        return transform(thisRef.valueOrDefault(property.name, defaultValue))
    }
}

private class ListConfigProperty<T : Any>(
    private val defaultValue: List<String>,
    private val transform: (List<String>) -> T
) : ReadOnlyProperty<ConfigAware, T> {
    override fun getValue(thisRef: ConfigAware, property: KProperty<*>): T {
        return transform(thisRef.valueOrDefaultCommaSeparated(property.name, defaultValue))
    }
}

It would be nice to use default values intead of overloads but in this case it's not possible. But, for the consumer, they are the same.

[marschwar]

I really like the idea, especially after renaming them to config, listConfig and fallbackConfig.

[marschwar]

Thank you for pointing out that we need to make sure that the transformer is only evaluated once. I adapted the code from LazyRegex to the config property delegate.

[picklebento]

Not something in this PR, but I will file an issue myself on changing the fallback value of having type () -> T instead of T - The benefit is that if the value is present, we do not need to initialize the fallback property.

[BraisGabin]

Is it worth it? We will be initializing booleans, strings, List. They will have more less the same cost to instantiate than the lambda itself and we will need to run the lambda too.

[marschwar]

On top of that please consider that We have to extract the default value for the documentation without the benefit of a BindingContext. This might end up really complicated.

[picklebento]

Is it worth it? We will be initializing booleans, strings, List.

I am looking for semantic correctness: If a property exists, we shouldn't read from the fallback property at all.

On top of that please consider that We have to extract the default value for the documentation without the benefit of a BindingContext. This might end up really complicated.

I don't fully get the benefit of a BindingContext, would you mind elaborate it more?

[marschwar]

Never mind. Reading your comment on my mobile I thought you were talking about something else. You were talking about something like this? I could add it to this PR or feel free to create an issue afterwards.

    override fun doGetValue(thisRef: ConfigAware, property: KProperty<*>): U {
        val fallbackValueSupplier = { getValueOrDefault(thisRef, fallbackPropertyName) { defaultValue } }
        return transform(getValueOrDefault(thisRef, property.name, fallbackValueSupplier))
    }

[picklebento]

👍 Yes that's what I meant.

[marschwar]

I am afraid I can't find a solution without actually invoking the supplier in getValueOrDefault which makes this exercise useless as we evaluate the fallback no matter what. I rather keep it the way it is if that is ok.

[picklebento]

No problem. We can table this for now

[picklebento]

Related to val needsQuotes = declaredTypeOrNull in TYPES_THAT_NEED_QUOTATION_FOR_DEFAULT, should we add quotes only when the source type is String?

I imagine the default value should be false without quotes.

[marschwar]

You are right. The problem here is that without a binding context or reflection I don't now how to reliably determine if the default value is a string or not. This is "best effort" by looking at the property type which (with transformi) might be something else.

[picklebento]

One possible alternative is using reified types for ConfigProperty.

[marschwar]

Is there anything that needs to be done here before merging? I would like to continue with #3670 if possible.

[picklebento]

All good. Thank you for the groundwork.