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

Source parser and stream utils #117

Merged
merged 12 commits into from
Apr 21, 2023
Merged

Source parser and stream utils #117

merged 12 commits into from
Apr 21, 2023

Conversation

bishiboosh
Copy link
Collaborator

The main motivation for this PR is to replicate what's done in https://github.com/Kotlin/kotlinx.serialization/blob/master/formats/json/jvmMain/src/kotlinx/serialization/json/JvmStreams.kt, because I'm using ktoml currently in an Android app and using streams directly without copying them to memory first would be a huge advantage to me.

The modifications done to the lib are the following:

  • change parseStringsToTomlTree to use a sequence instead of a list.
  • add a ktoml-source module, that parses directly from an okio Source
  • use this module in ktoml-file, allowing to avoid copying the file to memory while parsing it
  • adding JVM-only methods allowing to parse InputStream

I realize that this introduces JVM-only code, so I could spin the stream-related methods into a new project if needs be.

Copy link
Owner

@orchestr7 orchestr7 left a comment

Choose a reason for hiding this comment

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

Generally, looks good and should not affect a lot of people. But API changed, so we will need to take it at least into the minor release. May be into the 0.3.0 (yeah, let's finally release it)

/**
* Read from source one line at a time and passes the lines to the [decoder] function.
*/
public inline fun <T> Source.useLines(decoder: (Sequence<String>) -> T): T {
Copy link
Owner

Choose a reason for hiding this comment

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

is it possible to rename it to something more clear? :)
readAndDecodeLines or something like this?

@orchestr7
Copy link
Owner

May be you can also add some small example to the readme? You have added new logic to the API, that can be useful for users.

I fixed diktat/detekt code style issues and fixed some small typos

@bishiboosh
Copy link
Collaborator Author

@akuleshov7 I'll try to look at your comments today, but I may have to delay until next week, I'm a little bit swamped right now

@orchestr7
Copy link
Owner

orchestr7 commented May 5, 2022

@bishiboosh, you have been waiting for a review for a half of a year (sorry), so I do not think it is a problem for me to wait several week😅

@orchestr7 orchestr7 self-assigned this May 15, 2022
@orchestr7
Copy link
Owner

orchestr7 commented Feb 23, 2023

Only today I have noticed that this PR was opened in the same day when this awful war started exactly one year ago.
A year has passed already and still it has not ended, oh :(

I have opened #188 to finalise this PR: we need also to think how to reduce the number of affected users of the library.

� Conflicts:
�	gradlew.bat
�	kotlin-js-store/yarn.lock
�	ktoml-core/src/commonMain/kotlin/com/akuleshov7/ktoml/Toml.kt
�	ktoml-core/src/commonMain/kotlin/com/akuleshov7/ktoml/parsers/TomlParser.kt
�	ktoml-core/src/commonTest/kotlin/com/akuleshov7/ktoml/decoders/multiline/BasicMultilineStringDecoderTest.kt
�	ktoml-file/build.gradle.kts
�	ktoml-file/src/commonMain/kotlin/com/akuleshov7/ktoml/file/TomlFileReader.kt
�	ktoml-file/src/commonMain/kotlin/com/akuleshov7/ktoml/file/TomlFileWriter.kt
�	ktoml-file/src/commonTest/kotlin/com/akuleshov7/ktoml/file/TomlFileParserTest.kt
�	ktoml-source/src/jvmTest/resources/com/akuleshov7/ktoml/source/class_cast_regression1.toml
@orchestr7 orchestr7 linked an issue Apr 21, 2023 that may be closed by this pull request
@orchestr7 orchestr7 merged commit dedee03 into orchestr7:main Apr 21, 2023
@orchestr7
Copy link
Owner

orchestr7 commented Apr 21, 2023

And finally, after more than a year, I have merged conflicts 🥳
@bishiboosh thank you for this PR and sorry, that it took so long

@@ -4,8 +4,8 @@
"PACKAGE_NAME_INCORRECT_PATH")

object Versions {
const val KOTLIN = "1.6.21"
const val KOTLIN = "1.8.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

1.8.20?

Copy link
Owner

Choose a reason for hiding this comment

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

good note, thanks, updated to 1.8.21

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.

Migrate the decoding to sequences instead of collections
3 participants