Closed aSemy closed 12 months ago
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:
:lib
- the Kotlin Multiplatform library (essentially the code in my initial comment):app
- a Kotlin/JVM CLI app.In order to support backwards compatibility the UrlEncoder
could remain in :app
, while the encode/decode functions could be moved to a new class - named UrlEncoderSupport
? UrlEncoderLib
?
I'd like this change because I wanted to use UrlEncoder
in a library, and I have no need for a main function. And also it's useful to be able to create a single instance and re-use it rather than continuously passing in the same arguments.
@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:
When moving to Kotlin Multiplatform, the artifact with have the target suffixed on the end of it.
Now for Gradle that doesn't matter, because Gradle can automagically determine the correct usage. However, Maven isn't as clever and so Maven users will have to add a dependency using
net.thauvin.erik:urlencoder-jvm:1.3.0
It's possible to try and hack around to avoid this, but I think it's risky and requires maintenance.
On the other hand, while requiring Mavan users add -jvm
is a bit annoying, it's a small change, and Maven users are probably used to it from other Kotlin Multiplatform projects.
So what do you think? Update the README or try and update the POM?
Currently the tests use JUnit, which is JVM only. There are two options:
So what do you think? Update the README or try and update the POM?
Just update the documentation.
- Currently the tests use JUnit, which is JVM only. There are two options:
- Don't update the tests. Only the JVM will be tested.
- Update the tests to use a multiplatform-compatible test library. kotlin-test is the most similar to JUnit. Kotest is another option, but it would require a much larger refactoring.
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 sonar
task is working, but the koverXmlReport
task is always being skipped. Any ideas what's going on there? Without the report, sonar won't work properly.
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 :app
and :lib
, but it's skipping the root project because there aren't any tests (which makes sense). Maybe Sonar needs the report to be aggregated? I can try setting up an aggregated Kover XML report.
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!
Currently the tests use JUnit, which is JVM only. There are two options:
- Don't update the tests. Only the JVM will be tested.
- Update the tests to use a multiplatform-compatible test library. kotlin-test is the most similar to JUnit. Kotest is another option, but it would require a much larger refactoring.
Have you figured out how to use parameters in test in kotlin.test
? Looks like Kotest
supports that directly.
Have you figured out how to use parameters in test in
kotlin.test
? Looks likeKotest
supports that directly.
Looking that tests, a simple array loop would work with minimal changes. Do you want me to refactor the tests with Kotlin.test
or you have something else in mind?
@aSemy I converted the tests to kotlin.test
.
@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.
Have you figured out how to use parameters in test in kotlin.test? Looks like Kotest supports that directly.
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
functions.
@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 just checked out master and ran ./gradlew check
, and I can see the XML report for :app
is generated
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.
I just checked out master and ran
./gradlew check
, and I can see the XML report for:app
is generated
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:
* What went wrong:
Could not determine the dependencies of task ':urlencoder-app:dokkaJavadoc'.
> Empty collection can't be reduced.
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 README
and make sure it is accurately reflecting the multi-platform options, etc.
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 :)
> Empty collection can't be reduced.
That looks like this issue https://github.com/Kotlin/dokka/issues/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.
Also, could you look at the README and make sure it is accurately reflecting the multi-platform options, etc.
Sure, I'll take a look. The instructions for Maven users should be updated, because they'll be required to add a -jvm
suffix.
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.
@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 ./gradlew check
worked for me on Linux.
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.
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 e5cb0bd9035632da0ee0531673f9e43359ed743c
I still can't publish a snapshot:
❯ gradle publish
executing gradlew instead of gradle
Type-safe project accessors is an incubating feature.
> Task :urlencoder-lib:dokkaJavadoc FAILED
FAILURE: Build failed with an exception.
* What went wrong:
Execution failed for task ':urlencoder-lib:dokkaJavadoc'.
> Could not resolve all files for configuration ':urlencoder-lib:dokkaJavadocPlugin'.
> Resolved 'com.soywiz.korlibs.korte:korte-jvm:2.7.0' which is not part of the dependency lock state
> Resolved 'org.jetbrains.dokka:javadoc-plugin:1.8.20' which is not part of the dependency lock state
> Resolved 'org.jetbrains.dokka:kotlin-as-java-plugin:1.8.20' which is not part of the dependency lock state
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?
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 can take a look, but what happened? What errors did you see?
I removed the lock files and I got this when running check
:
Could not determine the dependencies of task ':kotlinNpmInstall'.
> Could not resolve all dependencies for configuration ':urlencoder-app:jsNpmAggregated'.
> Locking strict mode: Configuration ':urlencoder-app:jsNpmAggregated' is locked but does not have lock state.
Which got me quite puzzled.
did you also remove the config that enables the lockfiles?
did you also remove the config that enables the lockfiles?
@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:
Execution failed for task ':urlencoder-lib:generateMetadataFileForLinuxX64Publication'.
> java.io.FileNotFoundException: /home/erik/dev/kotlin/urlencoder/urlencoder-lib/build/classes/kotlin/linuxX64/main/klib/urlencoder-lib.klib (No such file or directory)
when trying to publish
.
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
urlencoder-lib/src/commonMain/kotlin/empty.kt
urlencoder-app/src/commonMain/kotlin/empty.kt
2. convert the project to multiplatform
@aSemy What does that entitle?
- convert the project to multiplatform
@aSemy What does that entitle?
Basically the changes I shared right at the top. In short: replace JVM only types and functions with Kotlin Multiplatform equivalents.
I think it's easier to look at the code differences, so I made a PR: https://github.com/ethauvin/urlencoder/pull/10
@aSemy Success! I was finally able to publish a snapshot
I think it's publishing too many things. Things like urlencoder-lib
or urlencoded-app
. Thoughts?
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 url-encoder
artifact. Maven users will have to specify the -jvm
suffix though.)
urlencoder-app-jvm
urlencoder-app
urlencoder-lib-js
urlencoder-lib-jvm
urlencoder-lib-linuxx64
urlencoder-lib-mingwx64
urlencoder-lib
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.
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
url-encoder
artifact. Maven users will have to specify the-jvm
suffix though.)
Okay, that makes sense.
urlencoder-app-jvm urlencoder-app urlencoder-lib-js urlencoder-lib-jvm urlencoder-lib-linuxx64 urlencoder-lib-mingwx64 urlencoder-lib
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.
Yeah, we'll have to do that. I don't use MacOS products anymore. I'll work on it.
Thanks!
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.
@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 net.thauvin.erik.urlencoder
so everything is in one directory in Maven. Thoughts?
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.
I'm thinking of changing the package id/group to net.thauvin.erik.urlencoder so everything is in one directory in Maven. Thoughts?
It makes sense to me 👍, it certainly looks more tidy.
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.
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.
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