Kotlin language port - multiplatform #366

apatrida · 2019-11-17T20:56:44Z

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.

…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.

googlebot · 2019-11-17T20:56:52Z

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 @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

apatrida · 2019-11-17T20:57:52Z

@googlebot I signed it!

googlebot · 2019-11-17T20:57:55Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

…t repeated.

…dant upper case calls

zongweil

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

zongweil · 2019-11-17T21:52:03Z

kotlin/src/commonMain/kotlin/com/google/openlocationcode/CharBuffer.kt

@@ -0,0 +1,24 @@
+package com.google.openlocationcode


What's the purpose of this class? Why use this as opposed to a string directly?

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.

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.

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

kotlin/src/commonTest/kotlin/com/google/openlocationcode/DecodingTest.kt

apatrida · 2019-11-18T03:02:40Z

@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.

apatrida · 2019-11-18T03:04:18Z

@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

apatrida · 2019-11-18T17:09:08Z

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.

apatrida · 2019-11-18T17:30:36Z

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:

…ase.

…eady be run for Java during testing.

…finition yet) rough in JS build for browsers as sample settings in Gradle.

zongweil

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.

zongweil · 2019-11-20T03:05:35Z

kotlin/src/commonMain/kotlin/com/google/openlocationcode/CharBuffer.kt

@@ -0,0 +1,24 @@
+package com.google.openlocationcode


I see that StringBuilder has a setCharAt method. Is there a reason we can't use that?

zongweil · 2019-11-20T03:06:57Z

kotlin/build.gradle

+        nodejs()
+    }
+
+//    js {


Why are these commented out? If they're not needed, can you just remove them to keep this file clean?

@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.

@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.

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?

apatrida added 2 commits November 17, 2019 14:50

TODO added to Gradle configuration for publishing, also change root p…

358d50a

…roject name to not include "common" since that would be added by the module.

googlebot added the cla: no label Nov 17, 2019

apatrida changed the title ~~Kotlin language port~~ Kotlin language port - multiplatform Nov 17, 2019

googlebot added cla: yes and removed cla: no labels Nov 17, 2019

apatrida mentioned this pull request Nov 17, 2019

Kotlin Translation #259

Open

apatrida added 2 commits November 17, 2019 15:11

incorporate PR 362 for Java version into Kotlin port, so the bug isn'…

335357f

…t repeated.

incorporate PR 158 for Java version into Kotlin port, to remove redun…

911dac6

…dant upper case calls

zongweil requested changes Nov 17, 2019

View reviewed changes

apatrida added 2 commits November 18, 2019 10:57

resolve comments in PR google#366

eca5563

Change CharBuffer to internal class and document it Fix documentation in comments of OpenLocationCode Fix imports that are .* to be explicit.

remove Gradle files

dc46ee9

start on README for setup/build

apatrida added 2 commits November 18, 2019 11:09

update .gitignore to exclude Gradle wrapper files.

ba03b09

setup maven publishing plugin for Gradle build, add notes about build…

d62efe8

…ing to the README, and add metadata publishing Gradle feature flag so that cross platform builds can build the generic root artifact.

apatrida requested a review from zongweil November 18, 2019 17:31

apatrida added 3 commits November 18, 2019 11:41

update README to talk about other possible platforms from same code b…

bfae388

…ase.

disable running the Java benchmark when the common benchmark will alr…

585b42a

…eady be run for Java during testing.

setup Nodejs build for testing purposes (no publishing, no package de…

b7ed512

…finition yet) rough in JS build for browsers as sample settings in Gradle.

zongweil requested changes Nov 20, 2019

View reviewed changes

Merge branch 'main' into kotlin

fdcd62f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kotlin language port - multiplatform #366

Kotlin language port - multiplatform #366

apatrida commented Nov 17, 2019 •

edited

googlebot commented Nov 17, 2019

apatrida commented Nov 17, 2019

googlebot commented Nov 17, 2019

zongweil left a comment

zongweil Nov 17, 2019

apatrida Nov 18, 2019

apatrida Nov 18, 2019

zongweil Nov 20, 2019

apatrida commented Nov 18, 2019

apatrida commented Nov 18, 2019

apatrida commented Nov 18, 2019

apatrida commented Nov 18, 2019

zongweil left a comment

zongweil Nov 20, 2019

zongweil Nov 20, 2019

apatrida Dec 3, 2019

apatrida Dec 3, 2019

zongweil Dec 7, 2019

Kotlin language port - multiplatform #366

Are you sure you want to change the base?

Kotlin language port - multiplatform #366

Conversation

apatrida commented Nov 17, 2019 • edited

googlebot commented Nov 17, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

apatrida commented Nov 17, 2019

googlebot commented Nov 17, 2019

zongweil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apatrida commented Nov 18, 2019

apatrida commented Nov 18, 2019

apatrida commented Nov 18, 2019

apatrida commented Nov 18, 2019

zongweil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apatrida commented Nov 17, 2019 •

edited