feat(common): implement collection interfaces for YamlList and YamlMap - #24
feat(common): implement collection interfaces for YamlList and YamlMap#24WhiredPlanck wants to merge 1 commit into
Conversation
|
Hi @WhiredPlanck! Could you please share your use-case? Currently I can't accept this PR since it's breaking equals/hashCode: And breaking API-compatibility. |
|
I want to explain properly why I can't take this, because the goal itself is reasonable and I'd like to land something here. Short version: I don't think 1. The equality break (recap)As I mentioned above, data class YamlList(private val items: List<String>, val path: String) : List<String> by items
val plain = listOf("a")
val node = YamlList(plain, "/root")
node == plain // false
plain == node // true
hashSetOf<List<String>>(plain).contains(node) // falseThat violates the 2. Even with equality fixed, half of the inherited API would be deadThis is the part that convinced me the approach is a dead end. The public data class YamlScalar(
val content: String,
override val path: YamlPath,
val plain: Boolean = true,
) : YamlNode(path)
Callers would still have to use the existing
3. Why this works for
|
|
Sorry for late reply. The motivation I make this PR is that I think the following use cases are a little verbose: If I want to access YamlList or YamlMap like a plain list or a plain map, I need: val transformedMap1 = node.yamlMap.entries.map { ... }
// even more verbose
val transformedMap2 = node.yamlMap.entries.entries.associate { ... }
val transformedList = node.yamlList.items.map { ... }For historical and compatible reasons, I must parse my configuration manually from yaml nodes. So I think If I can access YamlList and YamlMap like following examples, that can be more neat: val transformedMap1 = node.yamlMap.map { ... } // no entries, just like a plain map
val transformedMap2 = node.yamlMap.entries.associate { ... } // no one more entries to make one confuse
val transformedList = node.yamlList.map { ... } // no items, just like a plain list |
|
@WhiredPlanck yeah, I don't see how to provide meaningful improvement here without breaking existing contract. Implementing |
I'm making |
|
@WhiredPlanck because |
I check the definition and find that: For public inline fun <K, V, R> Map<out K, V>.map(transform: (Map.Entry<K, V>) -> R): List<R> {
return mapTo(ArrayList<R>(size), transform)
}
public inline fun <K, V, R, C : MutableCollection<in R>> Map<out K, V>.mapTo(destination: C, transform: (Map.Entry<K, V>) -> R): C {
for (item in this)
destination.add(transform(item))
return destination
}For public inline fun <T, R> Iterable<T>.map(transform: (T) -> R): List<R> {
return mapTo(ArrayList<R>(collectionSizeOrDefault(10)), transform)
}
@IgnorableReturnValue
public inline fun <T, R, C : MutableCollection<in R>> Iterable<T>.mapTo(destination: C, transform: (T) -> R): C {
for (item in this)
destination.add(transform(item))
return destination
}They almost share the same transform implementation expect the initial destination: for (item in this)
destination.add(transform(item))
return destinationUPDATED: But If just implement @PublishedApi
internal fun <T> Iterable<T>.collectionSizeOrDefault(default: Int): Int = if (this is Collection<*>) this.size else defaultI think Kotlin implements the almost same extensions for |
Make YamlList and YamlMap can perform like JsonArray and JsonObject in official JSON format support in kotlinx.serialization.
All tests pass on local, spotless rules applied.