ajalt / clikt

Multiplatform command line interface parsing for Kotlin
https://ajalt.github.io/clikt/
Apache License 2.0
2.53k stars 120 forks source link

Feature request: `associateBy` (or some way of returning other `Map<K, V>` types when using `associate`) #529

Closed FelixZY closed 3 months ago

FelixZY commented 3 months ago

Background

I've found myself in a situation where I want the user to provide a Map<Long, MyEnum> via the cli, something like

myapp --my-map 1=duck 2=dog 5=cat

However, there seems to be no native support for other associate types than Map<String, String> in Clikt.

What I looked for

Initially, I was thinking it might be possible to leverage a function similar to the standard library's associateBy. However, Clikt only exposes the associate function which does not fit my needs.

I then found the transformAll, transformValue and transformEach properties, but they were all vals and did not seem intended for me to change after calling associate.

Finally, I looked at the Clikt source code. Based on this, I was able to come up with a solution similar to this:

class Cli : NoOpCliktCommand() {
    val myMap: Map<Long, MyEnum> by option("--my-map")
        .splitPair("=")
        .convertKeys { /* ... */ }
        .convertValues { /* ... */ }
        .multiple()
        .toMap()

    fun <EachT, ValueT, NewEachT> NullableOption<Pair<EachT, ValueT>, Pair<EachT, ValueT>>.convertKeys(
        transform: OptionCallTransformContext.(EachT) -> NewEachT,
    ) = convert {
        transform(it.first) to it.second
    }

    fun <EachT, ValueT, NewValueT> NullableOption<Pair<EachT, ValueT>, Pair<EachT, ValueT>>.convertValues(
        transform: OptionCallTransformContext.(ValueT) -> NewValueT,
    ) = convert {
        it.first to transform(it.second)
    }
}

Unfortunately, this means I cannot use the associate sugar anymore. Also, I'm forced to run my own validations and miss out on nice error messages etc. provided by Clikt natively.

Proposal

I propose introducing a custom associateBy function similar to this:

class Cli : NoOpCliktCommand() {
    val myMap: Map<Long, MyEnum> by option("--my-map")
        .associateBy(valueTransform = { it.enum<MyEnum>() }) {
            it.long()
        }

    fun RawOption.associateBy(
        delimiter: String = "",
        valueTransform: (RawOption) -> /* ??? */ = { it },
        keyTransform: (RawOption) -> /* ??? */
    ) =
        // Note: pseudocode!
        splitPair(delimiter)
            .convertKeys { keyTransform(it.toOption().required()) }
            .convertValues { valueTransform(it.toOption().required()) }
            .multiple()
            .toMap()
}

There is a slight deviation from the standard library's associateBy in that we still call splitPair under the hood and do not ask the user to split the string themselves. However, I think this is acceptable sugar that additionally allows us to keep our key and value transforms short and to the point.

What I really like about this solution is that we can rely on the ready-made option transforms inside keyTransform/valueTransform, making it trivial to construct non-Map<String, String> maps.


An alternative is to continue with associate and add e.g. keyOptions and valueOptions like this:

class Cli : NoOpCliktCommand() {
    val myMap: Map<Long, MyEnum> by option("--my-map")
        .associate()
        .keyOptions { it.long() }
        .valueOptions() { it.enum<MyEnum>() }

    fun OptionWithValues<*, *, *>.keyOptions(
        transform: (RawOption) -> /* ??? */
    ) = TODO()

    fun OptionWithValues<*, *, *>.valueOptions(
        transform: (RawOption) -> /* ??? */
    ) = TODO()
}

However, I think this might be problematic as the user can now chain .keyOptions().keyOptions().keyOptions().keyOptions() or accidentally lose values after the associate step, e.g. if they define a constant return value for keyOptions. I also think this might be harder to implement.

Finally, I think associateBy is better because key deduplication is more of an "expected" side effect of associate/associateBy than keyOptions IMO.

ajalt commented 3 months ago

The easiest way to do this yourself right now would be

splitPair().convert { (k, v) -> k.toWhatever() to v.toWhatever() }.multiple().toMap()

And we could definitely add the associate* functions that are sugar for that.

Using clikt transforms to do the conversion sounds cool, but as you've seen it makes the API more complicated. I also think the error messages from that could be confusing, since they wouldn't mention the fact that they were part of a map.

JakeWharton commented 1 month ago

Just came to request this. Glad to see it'll be in the next release!