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

Add integration with kotlinx-io library #2707

Merged
merged 3 commits into from
Jun 13, 2024
Merged

Conversation

sandwwraith
Copy link
Member

@sandwwraith sandwwraith commented Jun 6, 2024

Integration is similar to Okio's one with functions encodeToSink, decodeFromSource, etc.

Fixes #253

Integration is similar to Okio's one with functions `encodeToSink`, `decodeFromSource`, etc.
@sandwwraith
Copy link
Member Author

sandwwraith commented Jun 6, 2024

The main question is how to name module/package:

kotlinx.serialization.json.kxio (as now) — a cryptic abbreviation
kotlinx.serialization.json.kotlinx.io — duplication of 'kotlinx'
kotlinx.serialization.json.io — not clear which IO we are referring to (we also have kotlinx.serialization.json.okio currently)

@fzhinkin
Copy link

fzhinkin commented Jun 6, 2024

The main question is how to name module/package:

kotlinx.serialization.json.kxio (as now) — a cryptic abbreviation

Personally, I like this one (most likely because I used to shorten the library name to kx-io), but indeed it looks a bit cryptic.

}

@Benchmark
fun decodeMicroTwitterKotlinxIoStream(): MicroTwitterFeed {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason, this particular benchmark shows worse results compared to Okio version. It doesn't block integration in any way, I'll check the root cause later.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a perf issue on the kotlinx-io size: an extension function to read code point values is defined only on Source and it performs a few actions to fill the inner buffer before decoding the code point. In Okio, that's a member function with a different (and optimized) implementation for Buffer.
kx-io should do the same: Kotlin/kotlinx-io#342.

@Dominaezzz
Copy link
Contributor

kotlinx.serialization.json.io — not clear which IO we are referring to

I think this is best. This library is under kotlinx so it's sensible for the default io to also be the kotlinx one.

Any duplicate mention of kotlinx, be it the abbreviation kx or the full thing kotlinx just seems like a waste of characters imo.

@DRSchlaubi
Copy link
Contributor

I think this is best. This library is under kotlinx so it's sensible for the default io to also be the kotlinx one.

Have to agree with that one, but what about simply including it like kx-datetime supports kx.ser out of the box, or does this run into the issue of compileOnly not working on non-jvm platforms?

@sandwwraith
Copy link
Member Author

@DRSchlaubi Yes, kotlinx-datetime actually uses api dependency for Js and Native.

@sandwwraith
Copy link
Member Author

Let's stick with kotlinx.serialization.json.io then.

formats/json-io/build.gradle.kts Outdated Show resolved Hide resolved
@fzhinkin fzhinkin self-requested a review June 12, 2024 08:25
@sandwwraith sandwwraith merged commit 08e604a into dev Jun 13, 2024
3 of 4 checks passed
@sandwwraith sandwwraith deleted the kotlinx-io-integration branch June 13, 2024 13:42
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 this pull request may close these issues.

5 participants