-
Notifications
You must be signed in to change notification settings - Fork 636
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
Add an encodeCollection extension #1749
Conversation
public inline fun Encoder.encodeCollection( | ||
descriptor: SerialDescriptor, | ||
collectionSize: Int, | ||
block: CompositeEncoder.() -> Unit | ||
) { | ||
val composite = beginCollection(descriptor, collectionSize) | ||
var ex: Throwable? = null | ||
try { | ||
composite.block() | ||
} catch (e: Throwable) { | ||
ex = e | ||
throw e | ||
} finally { | ||
// End structure only if there is no exception, otherwise it can be swallowed | ||
if (ex == null) composite.endStructure(descriptor) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the way that collections work, it would make sense to put the element iteration in the helper, not just the structure part. Why not use something like:
fun encodeCollection(
descriptor: SerialDescriptor,
collection: Collection<T>,
encodeElement: CompositeEncoder.(T) -> Unit
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can encode things using a collection structure that aren't collections themselves. The simplest example are arrays.
I don't understand the first part, are you talking about which file the extension lives in or something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ansman The first part refers to the idea that you would possibly want encodeCollection
to actually enforce/provide the expected semantics of a collection rather than leaving this to the caller. As to arrays, you can always overload the helper for array types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that idea but I don't want to reduce the flexibility of not enforcing a collection. Instead two extensions could be added (one that accepts a collection size and one that accepts a collection).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking care of it!
Could you please also make the new signatures right?
It's our mistake that we haven't marked the original signatures as crossinline
, so the ugly workaround with catch
is necessary. We aim to break it (on the source level) in 1.4.0, but these can be done in the desired way from the very beginning.
With crossinline
, it can be as straightforward as start() block() end()
without catch and finally blocks
Sure can! |
This is akin to encodeStructure but for collections. This fixes Kotlin#1748
75fb813
to
57bd71a
Compare
57bd71a
to
acdcabf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
This is akin to encodeStructure but for collections.
This fixes #1748