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.

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented package mapping for Java/Kotlin code generation #499

Implemented package mapping for Java/Kotlin code generation #499

netvl commented May 17, 2024 •

edited

netvl commented May 17, 2024

bioball left a comment

bioball May 20, 2024

netvl May 21, 2024

bioball left a comment

bioball Jun 4, 2024

bioball Jun 4, 2024

netvl Jun 5, 2024

bioball Jun 4, 2024 •

edited

netvl Jun 5, 2024

bioball Jun 4, 2024

netvl Jun 5, 2024

bioball Jun 4, 2024

netvl Jun 5, 2024

bioball Jun 4, 2024

bioball Jun 4, 2024

netvl Jun 5, 2024

bioball commented Jun 5, 2024

sgammon commented Jun 5, 2024

	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.

Implemented package mapping for Java/Kotlin code generation #499

Are you sure you want to change the base?

Implemented package mapping for Java/Kotlin code generation #499

Conversation

netvl commented May 17, 2024 • edited

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

bioball Jun 4, 2024 • edited

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

netvl commented May 17, 2024 •

edited

bioball Jun 4, 2024 •

edited