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

Rename string read/writeUtf8 methods #156

Conversation

fzhinkin
Copy link
Collaborator

@fzhinkin fzhinkin commented Jun 28, 2023

Closes #155; see the ticket for the rationale.

@fzhinkin fzhinkin requested review from qwwdfsad and shanshin June 28, 2023 15:25
@fzhinkin fzhinkin self-assigned this Jun 28, 2023
@fzhinkin fzhinkin changed the title Prototype preview rename string rw methods Rename string read/writeUtf8 methods Jun 28, 2023
@fzhinkin fzhinkin changed the base branch from prototype-preview to prototype-preview-byte-strings June 28, 2023 15:26
@fzhinkin fzhinkin marked this pull request as ready for review June 28, 2023 15:26
@whyoleg
Copy link
Contributor

whyoleg commented Jun 28, 2023

Question regarding Charset parameter position in functions. We have now:

  • Sink.writeString(string: String, charset: Charset, startIndex: Int = 0, endIndex: Int = string.length)
  • Source.readString(charset: Charset): String
  • Source.readString(byteCount: Long, charset: Charset): String
  • ByteString.decodeToString(charset: Charset): String
  • String.encodeToByteString(charset: Charset): ByteString

What is better:

  • what we have now, so Charset is last required argument (in case of writeString), and so f.e. can be even made optional (with default=Utf8) in future if multiplatform Charset will be supported
  • put Charset as first parameter in all functions? (as in writeString and readString(byteCount)

Im fine with both variants, just wanted to ask :)

@fzhinkin
Copy link
Collaborator Author

What is better:

  • what we have now, so Charset is last required argument (in case of writeString), and so f.e. can be even made optional (with default=Utf8) in future if multiplatform Charset will be supported

I didn't make it optional because there was already a function for writing UTF8 strings. And now, when it shares the name with Charset-based functions, we can encourage users to use a common function for UTF-8 strings instead of a JVM-specific one by requiring explicit charset value in the latter.

Possible future support for our own charsets in IO library is a great argument for supplying default value for the charset parameter, but charsets-support is not even on the horizon ATM, so I don't think it is worth preparing for it right now.

  • put Charset as first parameter in all functions? (as in writeString and readString(byteCount)

It looks strange to have a charset as a first parameter there. If we writing a string, then the string should be the first parameter and all other options controlling how it'll be written should follow it, not proceed.

@fzhinkin fzhinkin merged commit 457692a into prototype-preview-byte-strings Jun 29, 2023
@fzhinkin fzhinkin deleted the prototype-preview-rename-string-rw-methods branch June 29, 2023 09:58
fzhinkin added a commit that referenced this pull request Jun 30, 2023
- Implemented basic ByteString API
- Supported ByteString in kotlinx-io
- Changed ByteString::toString to always return full string representation
- Restructured unsafe API, move utf-8 conversion to base module
- Enabled JS target in bytestring module
- Made the code BCE-friendly
- Renamed readUtf8/writeUtf8 to readString/writeString (#156):

Currently, we use ByteString.decodeToString and String.encodeToByteString as names for conversion methods between String and ByteString, where non-parameterized functions convert to/from UTF-8, and JVM-specific extensions accept Charset.
At the same time, the core module uses different naming for methods reading/writing UTF-8 string and methods reading/writing strings using specific Charset:
- Source.readUtf8/Sink.writeUtf8 to work with UTF-8 strings
- Source.readString/Sink.writeString to work with strings using the given Charset on JVM.

The naming is inconsistent and it seems reasonable to unify read/write methods naming with what we have for ByteString.
We can use readString/writeString w/o charset for UTF-8 strings (as these are treated as default in the library) and same-titled JVM-specific extensions accepting a Charset.
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.

Unify ByteString/String conversions naming and read/write String methods naming from the core module
4 participants