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

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

Closed
FelixZY opened this issue Jul 11, 2024 · 2 comments · Fixed by #533

Comments

@FelixZY
Copy link

FelixZY commented Jul 11, 2024

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
Copy link
Owner

ajalt commented Jul 14, 2024

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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants