-
Notifications
You must be signed in to change notification settings - Fork 460
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
Kotlin language port - multiplatform #366
base: main
Are you sure you want to change the base?
Conversation
…tform projects. Port unit tests as well so they can be run on every platform in the future. Copy Java tests so that we validate that the API works from Java as expected (at least as far as the API used by the unit tests).
…roject name to not include "common" since that would be added by the module.
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
…dant upper case calls
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.
Thanks for your contribution, and especially for making sure your implementation is well-tested :). I'm not familiar with Kotlin, so my comments are more high-level.
-
Could you remove the Gradle binaries from the PR? Developers can install Gradle themselves, no need to include a specific binary in the library.
-
Can you add a README.md with info on how to build and call this library?
@drinckes to also take a look
@@ -0,0 +1,24 @@ | |||
package com.google.openlocationcode |
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.
What's the purpose of this class? Why use this as opposed to a string directly?
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.
This was originally a StringBuilder
in Java to expand as needed and allow setting new characters into existing positions. The common non-JVM lib of Kotlin doesn't have the ability to write at an index into its StringBuilder
and therefore I created an array wrapper to act like a mini StringBuilder, but I could look at it more carefully and maybe just use a String that is concatenated at each step, but it would be a lot of string copies on each append, and no ability to update existing values. It maybe just a fixed size CharArray
is fine, I just didn't go back to dig into the code more.
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.
I commented this class and made it internal. It can be removed in the future when Kotlin common allows conversion of the StringBuilder or String to a CharArray
without using experimental API.
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.
I see that StringBuilder has a setCharAt method. Is there a reason we can't use that?
kotlin/src/commonMain/kotlin/com/google/openlocationcode/OpenLocationCode.kt
Outdated
Show resolved
Hide resolved
kotlin/src/commonTest/kotlin/com/google/openlocationcode/DecodingTest.kt
Outdated
Show resolved
Hide resolved
@zongweil Yes, I can leave the Gradle scripts and remove the binaries, it will re-download when someone does a build. I just missed it on the ignore list. |
@zongweil for #2, how to build/call also should consider where we can publish the maven artifacts and get that added to the Gradle build so that the library can just be used as a normal dependency. I can only test that so far since I won't have rights to publish to the maven repo under this group ID. |
Change CharBuffer to internal class and document it Fix documentation in comments of OpenLocationCode Fix imports that are .* to be explicit.
start on README for setup/build
Gradle files are removed, now I'm looking at how this should be published to a repo and used. |
…ing to the README, and add metadata publishing Gradle feature flag so that cross platform builds can build the generic root artifact.
I setup publishing to local Maven repo, and this could be extended to public repos if we have specific configuration that can be setup for making this a public artifact. We could also build JavaScript version for Kotlin, and native libraries for Mac, Linux, Windows, and other platforms. But those would require build machines that are capable of building each target. More about this here: |
…eady be run for Java during testing.
…finition yet) rough in JS build for browsers as sample settings in Gradle.
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.
Thanks for addressing the comments and working on Maven setup.
We do already publish the Java implementation to MavenCentral. So developers using Java can already import it into their Maven/Gradle project. Can developers writing a Kotlin project use the existing Maven artifact? I'm trying to understand the advantages of publishing a separate artifact for Kotlin.
@@ -0,0 +1,24 @@ | |||
package com.google.openlocationcode |
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.
I see that StringBuilder has a setCharAt method. Is there a reason we can't use that?
nodejs() | ||
} | ||
|
||
// js { |
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.
Why are these commented out? If they're not needed, can you just remove them to keep this file clean?
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.
@zongweil the StringBuilder
method setCharAt
is not available on all platforms, and I wanted this to be a cross-platform build that others could extend to all targets available for Kotlin.
The commented out section can be removed, I was trying to decide to do a web build or not, and also setup other platforms. we can remove it and I can add them over time.
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.
@zongweil Kotlin can use the Java artifact only for the JVM build but not as a common code base to use across platforms. And it would be non-standard to publish common without the JVM and JS versions to the maven repo. I'll double check what is typical in other projects.
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.
Sounds good about StringBuilder. I didn't realize that setChatAt was only available for native :).
Please remove the commented out sections for cleanliness for now. It looks simple enough to add back in later.
Yes, if you could point me to another project that publishes Kotlin artifacts to MavenCentral, that would be helpful. It sounds like if someone is writing a Kotlin project and want to use plus codes in their project across all the supported platforms, they would need all the Kotlin artifacts published. Am I understanding that correctly?
This is a port of the Java code to Kotlin written as a common library so that additional platforms could be supported from the same codebase. It should compile to a common module, and a JVM module. In fact, it runs both common tests and the equivalent original Java tests to ensure that this could also be used from Java as a drop-in replacement API (minus one constructor change that was moved to a factory method, the rest should be API compatible).
Missing is Gradle publishing and making sure all maven artifacts are generated as expected (source, doc, bin jar). Could also add JavaScript tests, and native builds but that requires specific build machines which is more complicated than needed.