Skip to content
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

Add codegen setting governing Kotlin package #194

Merged
merged 1 commit into from
Feb 23, 2024

Conversation

sgammon
Copy link
Contributor

@sgammon sgammon commented Feb 18, 2024

Summary

Adds a setting to the Kotlin code generator which acts as a prefix for the target Kotlin package during codegen.

This has been filed on top of #192 to avoid conflicts.

Changelog

  • feat(gradle): kotlinPackage property in gradle plugin
  • feat(codegen): use kotlinPackage as prefix for kotlin codegen
  • fix: pin version of kotlinx.html and kotlinx.serialization
  • test(codegen): add tests for custom kotlin package
  • test(gradle): add tests for generating with custom kotlin package

holzensp
holzensp previously approved these changes Feb 22, 2024
Copy link
Contributor

@holzensp holzensp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me. Did @stackoverflow have any not-yet covered issues?

Comment on lines +562 to +572
"""
module my.mod

class Person {
name: String
age: Int
hobbies: List<String>
friends: Map<String, Person>
sibling: Person?
}
""",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised spotless doesn't flag this. Maybe auto-format (there are multiple sites)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is embedded Pkl inside Kotlin, which I don't think Spotless is aware of yet. Unless you mean the Kotlin around it? There are no errors from spotlessCheck, and spotlessApply doesn't change anything at this revision.

stackoverflow
stackoverflow previously approved these changes Feb 22, 2024
Copy link
Contributor

@stackoverflow stackoverflow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. You need to rebase main on your branch before we can merge.

@sgammon sgammon force-pushed the feat/issue-187 branch 2 times, most recently from f9ade2e to 0723834 Compare February 22, 2024 19:09
Adds a setting to the Kotlin code generator which controls the
target Kotlin package for codegen. Applied as a prefix to the
normal generated package name.

The current versions of the `kotlinx.serialization` and
`kotlinx.html` libraries are declared dynamically, which causes
a failure when refreshing dependencies. To avoid Kotlin metadata
build issues, these have been pinned.

- feat(codegen): use `kotlinPackage` as prefix for kotlin codegen
- feat(gradle): `kotlinPackage` property in gradle plugin
- test(codegen): add tests for custom kotlin package
- test(gradle): add tests for generating with custom kotlin package

Signed-off-by: Sam Gammon <sam@elide.ventures>
@sgammon
Copy link
Contributor Author

sgammon commented Feb 22, 2024

@stackoverflow / @holzensp Thank you for the review; I've adopted all suggestions and made all changes requested. There are no lockfile updates in this change anymore so we should be good there as well.

@sgammon
Copy link
Contributor Author

sgammon commented Feb 22, 2024

Wait, hold up; I've found some bugs on my fork. I'll add a few tests and push an update here to fix. Basically, Kotlinpoet is importing generated typealiases without the qualified package path. Easy fix

@sgammon sgammon dismissed stale reviews from holzensp and stackoverflow via b28452c February 22, 2024 20:27
@stackoverflow stackoverflow merged commit 7f404ff into apple:main Feb 23, 2024
4 checks passed
@bioball
Copy link
Contributor

bioball commented Feb 29, 2024

@sgammon: did you try setting a Pkl module name instead?

The normal way to solve this is to give your Pkl module a fully qualified name. For example:

module com.example.sgammon.MyModule

And this will generate package com.example.sgammon in the generated Kotlin code.

@sgammon
Copy link
Contributor Author

sgammon commented Feb 29, 2024

@bioball Yes, I'd noticed that, but this applies an unconditional prefix only for Kotlin, which might be needed in some corner cases. For example, in cases where there is no module involved, Pkl uses the root module, but JPMS forbids this.

bioball added a commit to bioball/pkl that referenced this pull request Feb 29, 2024
This reverts commit 7f404ff.

The package is derived from the module name.
Having `module com.example.Foo` in Pkl
will create Kotlin `package com.example`.
bioball added a commit that referenced this pull request Mar 4, 2024
This reverts commit 7f404ff.

The package is derived from the module name.
Having `module com.example.Foo` in Pkl
will create Kotlin `package com.example`.

Eventually, we may want to introduce a way to map
Pkl names to package names that provides finer
controls to the code generator.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants