-
Notifications
You must be signed in to change notification settings - Fork 257
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
Implemented package mapping for Java/Kotlin code generation #499
base: main
Are you sure you want to change the base?
Conversation
@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? |
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.
Generally looks on the right track!
* 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 |
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 should accept a structured Map<String, String>
; trace how externalProperties
and environmentVariables
get populated from the CLI args parser.
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 the pointer, I hoped that there is something like that!
|
||
private fun mapModuleName(name: String): kotlin.Pair<String, String> { | ||
val packageName = mapPackageName(name.substringBeforeLast('.', "")) | ||
val className = name.substringAfterLast('.').replaceFirstChar { it.titlecaseChar() } |
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.
Should maybe accept the name as-is?
val className = name.substringAfterLast('.').replaceFirstChar { it.titlecaseChar() } | |
val className = name.substringAfterLast('.') |
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 change would result in differences from the current behavior:
// current:
module foo.bar.baz -> package foo.bar; class Baz {}
// proposed:
module foo.bar.baz -> package foo.bar; class baz {}
That is, the class name would be lowercase, which is against Java naming conventions. This, in turn, would require users to always use a module name like foo.bar.Baz
if they want to get conventional Java names.
Ultimately, I think that even if we decide to do this, this should happen as a separate change, because it is only tangentially related to the original purpose of this PR.
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.
Canonically, such Pkl code should also have module foo.bar.Baz
if it's meant to be a class.
At the very least, though, if a package mapping is provided, we should probably not alter it.
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.
Canonically, such Pkl code should also have module foo.bar.Baz if it's meant to be a class.
This is the first time I hear about it :( I don't believe it is documented anywhere, and it is definitely not followed by many Pkl modules in the wild.
Different behavior when mapping is specified or not would be very confusing, IMO. Also, the mapping is specifically about packages, not module names.
I would prefer to limit the scope of the change and avoid doing unexpected changes in the existing behavior. But if you insist I can change it here.
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.
It's part of our style guide actually; see filename and module name
See #456 for motivation.
Closes #456