-
Notifications
You must be signed in to change notification settings - Fork 6
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
Support for Kotlin Multiplatform targets #1
Comments
HI @aSemy, Please submit a PR with your code changes and Gradle config. I'll review it and incorporate it. Thanks! E. |
@ethauvin Sure thing, I'll make a PR with my changes above and another with the Gradle config |
@aSemy Feel free to change/add to the README file too |
What do you think about supporting the CLI app? It will be difficult to support a multiplatform CLI (although not impossible). But that's not necessary - it's perfectly possible to keep the JVM CLI application while migrating the backing code to Kotlin Multiplatform. I think the best way to support this would be to split up the library into two subprojects:
In order to support backwards compatibility the I'd like this change because I wanted to use |
@aSemy I like that too. As long as we keep backward compatibility, I’m good with it. |
There are two more points to think about:
|
Just update the documentation.
I'm okay with kotlin-test, I've used it often. Thanks. |
@aSemy As I suspected, the sonar issue fixed itself when I merged the branches. I guess you were right, most likely a permission issue. |
@aSemy So the |
Good to hear that Sonar is still working! Next step is to migrate the project to a Kotlin/Multiplatform structure, while still keeping everything as JVM. After that, I can update the main to Multiplatform, and after that the tests. Regarding Kover: I had a quick look and I could see Kover was running for |
Ahh yes, it is an issue with the Sonar token not being available. When I look in the logs for the action run against the PR, the token isn't available: https://github.com/ethauvin/urlencoder/actions/runs/5181538553/jobs/9337181042?pr=5#step:7:12 While on an action run against master branch, it is available: https://github.com/ethauvin/urlencoder/actions/runs/5180967788/jobs/9335838718#step:7:12 |
@aSemy Last PR is working like a charm. Good job! |
great! |
Have you figured out how to use parameters in test in |
Looking that tests, a simple array loop would work with minimal changes. Do you want me to refactor the tests with |
@aSemy I converted the tests to |
@aSemy looks like the app tests are not processed by kover. |
Great work! Looks good. I think kotlin-test is a better idea than Kotest. It's more simple, but it's more stable, and it's more similar to JUnit.
There's no built-in support for parameterized tests. There are two options: either iterate over the test data in a single test (like you've done), or create a function that is called multiple times by @Test
fun `Main Decode 1`() = mainDecord("a test &" to "a%20test%20%26")
fun `Main Decode 2`() = mainDecord("!abcdefghijklmnopqrstuvwxyz%%ABCDEFGHIJKLMNOPQRSTUVQXYZ0123456789-_.~=" to "%21abcdefghijklmnopqrstuvwxyz%25%25ABCDEFGHIJKLMNOPQRSTUVQXYZ0123456789-_.%7E%3D")
private fun mainDecode(m: Pair<String, String>) {
val result: UrlEncoder.MainResult = processMain(arrayOf("-d", m.second))
assertEquals(m.first, result.output)
assertEquals(0, result.status, "processMain(-d ${m.second}).status")
} The benefit is that a single failing test will be easier to track down, because each test case is test directly. The downside is that it's a little more code. |
I think the way I did it is just fine; the parameters are included in the failure message which makes it really easy to track down too. Not much different from the way it worked with junit. |
Ah, good catch. I guess I need to let sonar know about it. |
@aSemy The PR has been merged. I can't seem to be able to publish the snapshot(s) at this point:
And I'm a little confused as to how you envisioned the publishing? The KVM lib and also the app? Let me know. I can easily set up an action to do test the snapshot publishing, if you need it. Also, could you look at the Let me know if I can do anything to help. Thanks, E. |
Good to see that there's some progress, even if it's not working perfectly :)
That looks like this issue Kotlin/dokka#3063. In this project we can resolve it by adding additional Kotlin targets (JS, and native targets). I'll make a new PR for this. Additionally, I'd recommend removing the generated Dokka from the Javadoc JAR. I don't think putting generated documentation Javadoc JAR is very useful, because I doubt many users are going to download, unzip, and view the generated docs. The sources JAR is much more useful, as this will be picked up by IDEs and contains the exact same documentation.
Sure, I'll take a look. The instructions for Maven users should be updated, because they'll be required to add a |
@aSemy I don't think that's a good idea. Maven Central, some IDEs, etc. are expecting the Javadoc jar to be there. I've merged the PRs, but haven't had a chance to test much yet, although |
Maven Central requires a Javadoc JAR is present, but it doesn't have any requirements on the content! I've used an empty Javadoc JAR in in a few of my projects. But it doesn't hurt to include some content in the JAR, so if you want to keep it then go for it :) I've made #9 to enable more OSes in the GitHub workflow, so that will help with testing. |
Thanks. I've fixed the workflow failing with Windows and JDK 17/20 with e5cb0bd I still can't publish a snapshot:
Any ideas? |
Hey @aSemy any ideas on the above? |
hey @ethauvin, I haven't looked into it but I guess it's a problem with Dokka and the Gradle lock files. I haven't used this combination, so I don't an answer to hand. It would either require some investigation, or you could ditch the lock file & Dokka requirements and attempt to add them back in later, after multiplatform support is stable? |
@aSemy I tried that, but I was getting errors with the JS stuff, etc. Do you mind taking a look at it? |
I can take a look, but what happened? What errors did you see? |
I removed the lock files and I got this when running
Which got me quite puzzled. |
did you also remove the config that enables the lockfiles?
urlencoder/buildSrc/build.gradle.kts Lines 13 to 22 in 32e2149
|
@aSeemy. I didn't, that helped. I also found a way to make dokka only generate docs for the jvm, but now I'm getting:
when trying to |
Haha the problems don't stop coming! Fortunately I recognise this one - it's because there are no sources in commonMain https://youtrack.jetbrains.com/issue/KT-52344/ There are two options
|
@aSemy What does that entitle? |
@aSemy Success! I was finally able to publish a snapshot I think it's publishing too many things. Things like |
Nice! I can see these artifacts, which look right to me. The app and lib were broken up, and there's a specific artifact for each Kotlin target. (The number of variants doesn't make a difference to Gradle users - thanks to some magic Gradle will be able to select the correct variant from the base
So actually it looks like some artifacts are missing - there aren't any macOS variants. Unfortunately publishing macOS variants requires a macOS machine. Do you have a Mac to hand? If not, we can set up publishing using a GitHub Action, since they have free Mac runners. |
Okay, that makes sense.
Yeah, we'll have to do that. I don't use MacOS products anymore. I'll work on it. Thanks! |
@aSemy I pushed the publish action. Could you please double-check that it is creating everything it should in the repo? You should be able to manually run it, if needed. Thanks! |
@aSemy I'm thinking of changing the package id/group to p.s. I ended up doing it, everything is there now: https://oss.sonatype.org/content/repositories/snapshots/net/thauvin/erik/urlencoder/ |
Looks like the snapshot version works! https://github.com/krzema12/snakeyaml-engine-kmp/pull/92/files Congrats on the Kotlin Multiplatform library! There might be an issue where UrlEncoder requires the latest Kotlin version - you might want to consider setting the Kotlin language level to 1.5 (here, I think), so UrlEncoder can be used more widely.
It makes sense to me 👍, it certainly looks more tidy. |
I set it to 1.6, as 1.5 is deprecated since January. Thanks for all your work on this; I don't think I could have done it on my own. |
### Summary Migrate UrlEncoder util to the now converted & published UrlEncoder library ethauvin/urlencoder#1 Co-authored-by: Piotr Krzeminski <git@krzeminski.it>
Hi 👋
I'd like to be able to use this library in a Kotlin Multiplatform project. Would you consider updating the library to support JVM, JS, and Native Kotlin compilation targets?
For now I've migrated it manually. I'm including the code so you are welcome to use it.
If you would like some help with the Gradle config, I can provide a PR.
Summary of changes:
safeChars
to class constructor parameterBitSet
withBooleanArray
ByteArray
operations to use Kotlin Multiplatform equivalentsPercentEscaper
)Character utils
The text was updated successfully, but these errors were encountered: