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

A trimmed-down API #136

Merged
merged 83 commits into from
Jun 27, 2023
Merged

Conversation

fzhinkin
Copy link
Collaborator

@fzhinkin fzhinkin commented Jun 12, 2023

Reduced kotlinx-io API shape by removing all niche and specific APIs.

Moved as many functions as possible to extensions, improved test coverage, and added documentation.

Docs generated for current branch are here: https://fzhinkin.github.io/kotlinx-io-dokka-docs-preview/

What changed compared to the previous preview version, in detail:

  • removed select API;
  • removed ByteStrings;
  • removed hash functions API;
  • added unsigned integer types support;
  • Sink::writeByte and Sink::writeShort now accept Byte and Short instead of Int;
  • Sink, Source, and Buffer no longer implement ByteChannels on JVM; functions returning wrappers around instances of these classes and implementing ByteChannels were provided instead;
  • introduced DelicateIoApi annotation, marked Sink::buffer, Sink::emitCompleteSegments, and Source::buffer with it;
  • enabled code coverage collection using Kover;
  • enabled Dokka for documentation;
  • turned on explicit API mode.

Some issues were not addressed yet:

What's out of this change's scope and will be resolved later:

  • performance;
  • ByteStrings;
  • replacement for cursor API.

Closes #132

fzhinkin added 30 commits June 12, 2023 14:10
- Removed byte strings and ByteString related APIs
- Moved some methods that were previously implemented as member functions to extensions, provided basic implementation
- Get rid of hash functions and select API
- Get rid of unsafe cursors
- Removed a lot of commented out code
- Enabled some previously disabled tests
- Added tests covering line reading
@whyoleg
Copy link
Contributor

whyoleg commented Jun 23, 2023

I think it's better to return the buffered source itself from the Source::buffered / Sink::buffered instead of introducing a deprecated method.

May be then it will be sufficient to do something like this:

public fun RawSource.buffered(): Source = when(this) {
    is Source -> this
    else      -> RealSource(this)
}

(same for sink)

fzhinkin added 2 commits June 23, 2023 11:26
Always print data as hex, don't try decoding it as text.
@swankjesse
Copy link

@whyoleg I recommendation against a feature to prevent multiple buffer wrappers. We were occasionally tempted to do this for Okio!

The runtime cost of redundant buffering is nearly zero. (This is a consequence of how data is moved and not copied between layers.)

And it creates quite surprising behaviour consequences. For example it makes code that forgets to call ‘emit()’ or ‘flush()’ work sometimes when it shouldn’t.

@fzhinkin
Copy link
Collaborator Author

@whyoleg I recommendation against a feature to prevent multiple buffer wrappers. We were occasionally tempted to do this for Okio!

The runtime cost of redundant buffering is nearly zero. (This is a consequence of how data is moved and not copied between layers.)

And it creates quite surprising behaviour consequences. For example it makes code that forgets to call ‘emit()’ or ‘flush()’ work sometimes when it shouldn’t.

For me, it seems like both modes are legit - one may want to always wrap a Source into another Source, at least for testing, while in some cases, it may add an overhead (yeah, copying segments between buffers is fast, but the cost is non zero).
I think we can leave old behavior for now and discuss alternatives in a separate issue.

@whyoleg
Copy link
Contributor

whyoleg commented Jun 23, 2023

@swankjesse after looking for a while on examples in kdoc of flush and emit in Okio - I'm now thinking, that you are right that it will be not really expected behaviour if we will just return same object.

Copy link
Contributor

@shanshin shanshin left a comment

Choose a reason for hiding this comment

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

Waiting for completion

@whyoleg
Copy link
Contributor

whyoleg commented Jun 23, 2023

@fzhinkin can you update preview documentation (https://github.com/fzhinkin/kotlinx-io-dokka-docs-preview) after all latest changes - it's much more easier to check all available public API there as members and extensions are there in one list and sorted by name ?

@fzhinkin
Copy link
Collaborator Author

@fzhinkin can you update preview documentation (https://github.com/fzhinkin/kotlinx-io-dokka-docs-preview) after all latest changes - it's much more easier to check all available public API there as members and extensions are there in one list and sorted by name ?

@whyoleg done, thanks for reminding me!

@fzhinkin fzhinkin requested a review from qwwdfsad June 26, 2023 10:18
@qwwdfsad qwwdfsad requested a review from shanshin June 26, 2023 10:28
Copy link
Collaborator

@qwwdfsad qwwdfsad left a comment

Choose a reason for hiding this comment

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

Great job!

Left a few minor comments, feel free to address them as you see fit

*/
fun cancel()
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seemed like this method was useful even without explicit asynchrony (and was, in fact, not related to coroutines-based cancellation); I might be mistaken though, anyway worth initiating the discussion

@fzhinkin fzhinkin merged commit 6a172a7 into prototype-preview Jun 27, 2023
@fzhinkin fzhinkin deleted the prototype-preview-trimmed-down-api branch June 27, 2023 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants