Implemented package mapping for Java/Kotlin code generation #499

netvl · 2024-05-17T03:06:00Z

See #456 for motivation.

Implement package mapping in Java codegen, add tests
Implement package mapping in Kotlin codegen, add tests
Propagate configuration to the Gradle plugin

Closes #456

netvl · 2024-05-17T03:06:42Z

@bioball - I've started the package mapping implementation, just wanted to show the overall direction before I start implementing it for Kotlin. What do you think?

bioball

Generally looks on the right track!

bioball · 2024-05-20T21:25:47Z

pkl-codegen-java/src/main/kotlin/org/pkl/codegen/java/CliJavaCodeGeneratorOptions.kt

+   * When this option is set, the mapping will be used to translate package names derived from
+   * module names to the specified package names.
+   */
+  val packageMapping: String? = null


This should accept a structured Map<String, String>; trace how externalProperties and environmentVariables get populated from the CLI args parser.

Thanks for the pointer, I hoped that there is something like that!

pkl-codegen-java/src/main/kotlin/org/pkl/codegen/java/JavaCodeGenerator.kt

bioball

This looks pretty good! I am thinking we should adjust how the mapping works, though (see comment in review)

bioball · 2024-06-04T15:04:27Z

pkl-codegen-java/src/main/kotlin/org/pkl/codegen/java/Main.kt

@@ -108,6 +109,18 @@ class PklJavaCodegenCommand :
      )
      .flag()

+  private val packageMapping: Map<String, String> by
+    option(
+      names = arrayOf("-m", "--package-mapping"),


We tend to be conservative about which flags get short flag names, because they quickly pollute the namespace.

I think this one should just be kept as a long flag name.

Suggested change

names = arrayOf("-m", "--package-mapping"),

names = arrayOf("--package-mapping"),

bioball · 2024-06-04T17:37:05Z

pkl-codegen-java/src/test/kotlin/org/pkl/codegen/java/JavaCodeGeneratorTest.kt

@@ -127,6 +129,9 @@ class JavaCodeGeneratorTest {
    }
  }

+  @TempDir
+  lateinit var tempDir: Path


Why was this moved to the test suite level?

Two reasons:

tempDir is used by helper methods below, which makes test code simpler compared to constant passing of tempDir as an argument.

Removed repetition because this parameter was present on lots of test methods.

In general, there is not much difference between a field and a method parameter for this use case. JUnit Jupiter always creates a fresh instance of the test class for each test, and it always populates the marked fields before the tests are run, so defining temp dir as a variable allows to simplify some of the helper methods logic and reduce duplication.

bioball · 2024-06-04T17:50:03Z

pkl-codegen-kotlin/src/test/kotlin/org/pkl/codegen/kotlin/KotlinCodeGeneratorTest.kt

+    )
+
+    // No more files.
+    assertThat(files).isEmpty()


Nit: I typically write these tests this way, because it's clearer what the assertion is and helps with the error message when it fails.

assertThat(files).containsOnlyKeys("foo", "bar", "bar") assertThat(files["foo"]).contains("something text") assertThat(files["bar"]).contains("something text")

This isn't a blocker, though, feel free to just keep this as-is.

I personally don't have much preference either; I chose the current style because this way, I can avoid repeating the map keys.

bioball · 2024-06-04T17:57:31Z

pkl-codegen-java/src/main/kotlin/org/pkl/codegen/java/JavaCodeGenerator.kt

@@ -872,6 +870,15 @@ class JavaCodeGenerator(
      } else key
    }
  }
+
+  private fun mapPackageName(name: String): String =
+    codegenOptions.packageMapping[name] ?: name


I think instead of doing an exact match, this should be a prefix match. That would make this functionality a lot more useful. Also, with an exact match, this seems a little brittle. If a new Pkl module gets added at some subpath, it will fail to match and thus not be mapped to a custom name.

Prefix everything:

mapOf("" to "com.foo.")

Prefix everything starting with "foo." to "com.foo":

mapOf("foo." to "com.foo.")

I think the logic should work like so:

Sort all the mapping entries by length (greatest first)

Find the first entry that matches by prefix

This way, the most specific prefix has greater precedence; the "com.foo" pattern beats the "com" pattern.

Yes, this definitely does make sense, and makes the feature more powerful and useful, thanks for the suggestion!

pkl-codegen-java/src/main/kotlin/org/pkl/codegen/java/JavaCodeGenerator.kt

bioball · 2024-06-04T18:06:13Z

pkl-gradle/src/test/kotlin/org/pkl/gradle/JavaCodeGeneratorsTest.kt

  private fun writePklFile() {
    writeFile(
      "mod.pkl", """
        module org.mod

        class Person {
          name: String
-          addresses: List<Address>
+          addresses: List<Address?>


Why was this changed?

Previously, there was two almost identical Pkl scripts here, whose difference was basically in this ?. The question mark was necessary, because apparently the code generated without this question mark fails to compile with some really strange error about annotations. I unified these two scripts to avoid repetition, and to make sure that the code can be compiled, I switched it to be nullable. I think it does not matter much here because we are not really concerned with the actual contents of the generated classes, we check that the Gradle plugin works and can be used to generate and compile code properly.

Ah, probably because the in-memory compiler didn't have access to our NonNull annotation. Gotcha, thanks for the explanation.

bioball · 2024-06-04T18:06:48Z

pkl-codegen-kotlin/src/main/kotlin/org/pkl/codegen/kotlin/Main.kt

+
+  private val packageMapping: Map<String, String> by
+    option(
+      names = arrayOf("-m", "--package-mapping"),


See comment earlier about short names

Suggested change

names = arrayOf("-m", "--package-mapping"),

names = arrayOf("--package-mapping"),

bioball · 2024-06-04T18:10:30Z

docs/modules/java-binding/partials/cli-codegen-options.adoc

+
+When you need the generated package names to be different from the default package names derived from Pkl module name prefixes, you can define a package mapping, where the key is the original Pkl module name prefix, and the value is its replacement. When you do, the generated code `package` declarations, as well as file locations, will be modified according to the mapping.
+
+Repeat this option to define multiple mappings. Keys must be valid Pkl module name prefixes. Values must be valid dot-separated package names.


Technically, keys can be anything; and if they don't match anything, they are ignored.

Suggested change

Repeat this option to define multiple mappings. Keys must be valid Pkl module name prefixes. Values must be valid dot-separated package names.

Repeat this option to define multiple mappings. Values must be valid dot-separated package names.

Also: our style here is to separate every sentence to be on their own line.

I thought I definitely saw some paragraphs with multiple sentences in one line, that's why I was not concerned about it. I will check again and fix it.

bioball · 2024-06-05T17:01:43Z

FYI @sgammon: you might be interested in this.

sgammon · 2024-06-05T19:49:49Z

@bioball yes, I very much am! thanks for the tag. I'd be happy to test this if helpful.

* Implemented prefix-based mapping instead of literal matching. * Removed single-letter name for the new CLI flag. * Fixed formatting.

Now it is possible to rename not just the package name, but the class name entirely.

bioball

This mostly looks good!

I have mostly nits, but one remaining change here is around the logic for deriving Java/Kotlin class names, where we don't rename yet again for class names that are explicitly renamed.

bioball · 2024-06-10T21:01:25Z

pkl-gradle/src/test/kotlin/org/pkl/gradle/KotlinCodeGeneratorsTest.kt

@@ -125,6 +125,9 @@ class KotlinCodeGeneratorsTest : AbstractTest() {
            sourceModules = ["mod.pkl"]
            outputDir = file("build/generated")
            settingsModule = "pkl:settings"
+            packageMapping = [
+              'org': 'foo.bar'


Suggested change

'org': 'foo.bar'

'org.': 'foo.bar.'

bioball · 2024-06-12T17:28:26Z

pkl-codegen-java/src/test/kotlin/org/pkl/codegen/java/JavaCodeGeneratorTest.kt

@@ -127,6 +129,9 @@ class JavaCodeGeneratorTest {
    }
  }

+  @TempDir
+  lateinit var tempDir: Path


bioball · 2024-06-12T17:47:55Z

pkl-codegen-java/src/main/kotlin/org/pkl/codegen/java/Main.kt

-        names = arrayOf("--package-mapping"),
-        metavar = "<module_prefix=package_prefix>",
+        names = arrayOf("--rename"),
+        metavar = "<module_prefix=class_prefix>",


Suggested change

metavar = "<module_prefix=class_prefix>",

metavar = "<old_name=new_name>",

bioball · 2024-06-12T17:49:24Z

pkl-codegen-kotlin/src/main/kotlin/org/pkl/codegen/kotlin/Main.kt

-        names = arrayOf("--package-mapping"),
-        metavar = "<module_prefix=package_prefix>",
+        names = arrayOf("--rename"),
+        metavar = "<module_prefix=class_prefix>",


Suggested change

metavar = "<module_prefix=class_prefix>",

metavar = "<old_name=new_name>",

bioball · 2024-06-12T17:52:21Z

pkl-codegen-java/src/test/kotlin/org/pkl/codegen/java/JavaCodeGeneratorTest.kt

+  }
+
+  @Test
+  fun `override package names based on the longest prefix`() {


Suggested change

fun `override package names based on the longest prefix`() {

fun `override names based on the longest prefix`() {

bioball · 2024-06-12T17:52:32Z

pkl-codegen-java/src/test/kotlin/org/pkl/codegen/java/JavaCodeGeneratorTest.kt

-      acc + JavaCodeGenerator(pklSchema, JavaCodegenOptions()).output
-    }
+  @Test
+  fun `override package names in a standalone module`() {


Suggested change

fun `override package names in a standalone module`() {

fun `override names in a standalone module`() {

bioball · 2024-06-12T18:13:49Z

pkl-codegen-java/src/main/kotlin/org/pkl/codegen/java/JavaCodeGenerator.kt

+    val packageName = mappedName.substringBeforeLast('.', "")
+    val className = mappedName.substringAfterLast('.').replaceFirstChar { it.titlecaseChar() }
+    return packageName to className
+  }


Now that this is a straight up rename, I think we should adjust this so that we don't then change the name again if it's been explicitly renamed.

I think the logic here can be adjusted to something like:

private fun mapModuleName(name: String): kotlin.Pair<String, String> { val (mappedName, isClassRenamed) = nameMapper.map(name) val packageName = mappedName.substringBeforeLast('.', "") val className = mappedName.substringAfterLast('.').let { segment -> if (isClassRenamed) segment else segment.replaceFirstChar { it.titlecaseChar() } } return packageName to className }

Where org.pkl.commons.NameMapper#map gets changed to:

fun map(sourceName: String): Pair<String, Boolean> { for ((sourcePrefix, targetPrefix) in sortedMapping) { if (sourceName.startsWith(sourcePrefix)) { val remainder = sourceName.substring(sourcePrefix.length) return targetPrefix + remainder to ((sourcePrefix.length - 1) > sourceName.lastIndexOf('.')) } } return sourceName to false }

Alternatively, org.pkl.commons.NameMapper#map just returns a package name and class name tuple.

bioball · 2024-06-12T18:16:07Z

pkl-codegen-java/src/test/kotlin/org/pkl/codegen/java/JavaCodeGeneratorTest.kt

@@ -101,7 +101,8 @@ class JavaCodeGeneratorTest {
      generateJavadoc: Boolean = false,
      generateSpringBootConfig: Boolean = false,
      nonNullAnnotation: String? = null,
-      implementSerializable: Boolean = false
+      implementSerializable: Boolean = false,
+      packageMapping: Map<String, String> = emptyMap()


Suggested change

packageMapping: Map<String, String> = emptyMap()

renames: Map<String, String> = emptyMap()

Do not titlecase the class name if the target name was explicitly requested by the user.

bioball

LGTM, good work, and thank you for the feature!

Implemented package mapping for Java code generation

9f0afbd

bioball reviewed May 20, 2024

View reviewed changes

netvl added 5 commits May 21, 2024 14:02

Fixed CLI options parsing for java codegen

3fd714a

Added path filtering to Java codegen

86bb912

Implemented support for package mappings in Kotlin codegen

da06219

Added support for package mappings in the Gradle plugin

ffe8aba

Updated documentation for CLI and Gradle plugins

da1ab23

netvl marked this pull request as ready for review May 30, 2024 20:23

bioball requested changes Jun 4, 2024

View reviewed changes

netvl added 2 commits June 7, 2024 20:47

Fixes per review

e211a49

* Implemented prefix-based mapping instead of literal matching. * Removed single-letter name for the new CLI flag. * Fixed formatting.

Implemented full class renaming

518e9c1

Now it is possible to rename not just the package name, but the class name entirely.

bioball requested changes Jun 12, 2024

View reviewed changes

netvl and others added 3 commits June 12, 2024 13:30

Fixes per review

efdb28d

Do not titlecase the class name if the target name was explicitly requested by the user.

Adjust docs and add tests for NameMapper, miscellaneous polish

3d7e411

Adjust source block style

66be706

bioball approved these changes Jun 12, 2024

View reviewed changes

bioball merged commit d7a1778 into apple:main Jun 12, 2024
4 checks passed

	names = arrayOf("-m", "--package-mapping"),
	names = arrayOf("--package-mapping"),


		When you need the generated package names to be different from the default package names derived from Pkl module name prefixes, you can define a package mapping, where the key is the original Pkl module name prefix, and the value is its replacement. When you do, the generated code `package` declarations, as well as file locations, will be modified according to the mapping.

		Repeat this option to define multiple mappings. Keys must be valid Pkl module name prefixes. Values must be valid dot-separated package names.

	metavar = "<module_prefix=class_prefix>",
	metavar = "<old_name=new_name>",

	fun `override package names based on the longest prefix`() {
	fun `override names based on the longest prefix`() {

	fun `override package names in a standalone module`() {
	fun `override names in a standalone module`() {

	packageMapping: Map<String, String> = emptyMap()
	renames: Map<String, String> = emptyMap()

Implemented package mapping for Java/Kotlin code generation #499

Implemented package mapping for Java/Kotlin code generation #499

Conversation

netvl commented May 17, 2024 • edited Loading

netvl commented May 17, 2024

bioball left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bioball 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

bioball Jun 4, 2024 • edited Loading

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

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

Choose a reason for hiding this comment

bioball commented Jun 5, 2024

sgammon commented Jun 5, 2024

bioball 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

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

bioball left a comment

Choose a reason for hiding this comment

netvl commented May 17, 2024 •

edited

Loading

bioball Jun 4, 2024 •

edited

Loading