square / okio

A modern I/O library for Android, Java, and Kotlin Multiplatform.
https://square.github.io/okio/
Apache License 2.0
8.72k stars 1.17k forks source link

Add EnumOptions as an exhaustive alternative to Options #1405

Closed kyay10 closed 5 months ago

kyay10 commented 6 months ago

This facilitates using when exhaustively on it.

swankjesse commented 5 months ago

Thanks for this.

kyay10 commented 5 months ago

No problem at all, I completely get it! It didn't take me that much time. To be honest, I saw one of your talks about okio, saw that Options were very rudimentary, and thought hey I could fix this in an afternoon.

Would you like me to open an issue for this, so that we can discuss possible implementations?

I can imagine you're very busy so of course it's not a priority at all. I just want to try some implementations out to see which would fit okio the best (I don't have extensive experience with the library so of course your feedback is much much appreciated).

swankjesse commented 5 months ago

Thanks for being accommodating. 😅

I spent my lunch time thinking this over. I really like your idea of a select() function that uses something friendlier than ints. Here’s the smallest API I can think of to satisfy that requirement:

class TypedOptions<T : Any>(
  values: Iterable<T>,
  encode: (T) -> ByteString,
) {
  internal val list = values.toList()
  internal val options = Options.of(*list.map { encode(it) }.toTypedArray<ByteString>())
}

fun <T : Any> BufferedSource.select(typedOptions: TypedOptions<T>): T? {
  return when (val index = select(typedOptions.options)) {
    -1 -> null
    else -> typedOptions.list[index]
  }
}

And here’s what it looks like in practice:

  @Test
  fun happyPath() {
    val durationOptions = TypedOptions(DurationUnit.entries) {
      it.name.encodeUtf8()
    }

    val buffer = Buffer().writeUtf8("SECONDS,HOURS,DAYS")
    assertThat(buffer.select(durationOptions)).isEqualTo(DurationUnit.SECONDS)
    buffer.readByte()
    assertThat(buffer.select(durationOptions)).isEqualTo(DurationUnit.HOURS)
    buffer.readByte()
    assertThat(buffer.select(durationOptions)).isEqualTo(DurationUnit.DAYS)
  }
swankjesse commented 5 months ago

Next steps: if the above works for you, I’ll add a couple tests & send a pull request!

kyay10 commented 5 months ago

Yep that absolutely works! Only nitpick I have would maybe be making the constructor just take the list of bytestrings instead, and making that an internal constructor, then having a "smart constructor" which is just a function that actually does the mapping business, so that there's no unnecessary lambda objects being created or stored around

swankjesse commented 5 months ago

Here it is! https://github.com/square/okio/pull/1417