feat: Add bare Kotlin type overloads for expression functions

[sargunv]

closes #193

[sargunv]

• feat: Add bare Kotlin type overloads for expression functions #196 👈 (View in Graphite)
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[westnordost]

seems to be missing for

• both switch (i.e. the fallback, but also secondary constructors for the case and condition
• all the comparison functions
• within

By the way, why is the constructor of Case and Condition private and then there is a function that is basically a constructor for it?

[sargunv]

It's just stylistic preference. I prefer to think of the expressions DSL as regular functions, and short-lived objects like Case and Condition are implementation details to get the typing and syntax right.

Additionally, Dokka shows documentation for functions on a different page from types, and this keeps documentation for the construct and also the branches on the same page.

[sargunv]

and similar to math, overloads for generic stops means writing a whole lot of overloads for many different types

[westnordost]

true... , but using literals for these constructs is pretty much the default case for these constructs.

[westnordost]

all overloads with bare types are missing for the math functions

[sargunv]

For math operators, it felt odd to me to have different types on two sides of the operator. Maybe it's okay though

[sargunv]

also for anything related to numbers with units, we'd write a lot of overloads for bare types (no unit, Dp, TextUnit, Duration, maybe Meters in the future). Seems not worth it?

[westnordost]

Well, not sure, IIRC the Android MapLIbre SDK does that. (For Floats only, of course)

[westnordost]

The stops for the interpolate expressions and step expressions don't have bare Kotlin overloads.

[sargunv]

With current typing (the Pair), either all stops will have to be Kotlin values, or all stops Expressions. Thoughts on doing something like case and condition (maybe stop?) so we can use different overloads on each stop?

[westnordost]

I think it is rather the default / usual case that no expressions are used, so the overload that doesn't allow any expressions for the stops would I think be the most used one.
Having data classes for step and interpolate i.e. stop(...) sounds fine to me, too. It's only marginally less convenient/more verbose than the to (and anyway, that is just a normal infix function, you could name the function that creates a Stop also like that.)

[sargunv]

I'd be wary of shadowing to so would want a different infix name, or just stop() like the others.

It's true that most stops/case/condition would be const, but also interpolate/switch is already a pretty big, verbose, construct, and so adding one const per stop doesn't hurt readability much (at least to my eye).

Compared to get and has which were small enough that the const dwarfed the get/has and made it confusing to read.

[westnordost]

it wouldn't really be shorter, and would be less transparemt El 27 de diciembre de 2024 16:55:29 CET, Sargun Vohra ***@***.***> escribió:

…

@sargunv commented on this pull request. > @@ -16,6 +16,11 @@ public operator fun <T : ExpressionValue> Expression<ListValue<T>>.get( index: Expression<IntValue> ): Expression<T> = FunctionCall.of("at", index, this).cast() +/** Returns the item at [index]. */ ***@***.***("getAt") +public operator fun <T : ExpressionValue> Expression<ListValue<T>>.get(index: Int): Expression<T> = + get(const(index)) + /** Returns whether this list contains the [item]. */ @JvmName("containsList") public fun <T : ExpressionValue> Expression<ListValue<T>>.contains( Adding an overload for List<String> sounds good to me if it doesn't shadow the regular Kotlin `contains`. If it does shadow, well I think we already have `const(listOf())` so it's not too bad. Should there be a `constList(varargs)`? -- Reply to this email directly or view it on GitHub: #196 (comment) You are receiving this because your review was requested. Message ID: ***@***.***>