-
Notifications
You must be signed in to change notification settings - Fork 50
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
Convert JVM target to use KMP #507
Convert JVM target to use KMP #507
Conversation
I have to drop working on this, sorry! Feel free to take over. |
Could you summarize what is still missing in this PR? Do you think it would require adjustments in snakeyaml-engine-kmp itself? |
@westnordost for sure the JVM-specific API, not sure what more. I recall not being able to get past some failures in unit tests. If some changes are needed in snakeyaml-engine-kmp, I'm the owner of it, so I'll be glad to help. Not sure if any changes will be needed. |
# Conflicts: # build.gradle.kts
I've picked this up and I've managed to get the existing tests passing 🎉. @krzema12 Would you be able to merge these PRs? |
Convert jvm to use kmp: implement reading/writing from Java input/output streams
Improvements for MarkedYamlEngineException, for use by Kaml in charleskorn/kaml#507 - make 'problem' required (matches existing usages) - make context/problems properties (they are used by Kaml) - fix problem/context order of ScannerException in ScannerImpl
Awesome, thanks @aSemy! Next step, on me: release snakeyaml-engine-kmp with your fix so that we can use it in this PR. |
https://github.com/krzema12/snakeyaml-engine-kmp/releases/tag/v2.7.2 @edit: I think the release is broken, see https://repo.maven.apache.org/maven2/it/krzeminski/snakeyaml-engine-kmp-js/2.7.2/ - almost no artifacts. That's why CI fails. I'm gonna try again. |
@charleskorn could you review please? |
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.
Sorry for the slow response to this, I've been a bit busy!
Thanks for working on this, this looks good overall.
…into convert-jvm-to-use-kmp
…ting an unnecessary buffer when encoding to an existing Sink
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.
Regarding the disabled JS tests:
browser {
testTask {
// TODO: enable once the tests work with Kotlin/JS.
enabled = false
}
}
nodejs {
testTask {
// TODO: enable once the tests work with Kotlin/JS.
enabled = false
}
}
I've implemented a workaround in Kotest, so nested tests should work.
So, please +1 and review that PR :)
No description provided.