-
Notifications
You must be signed in to change notification settings - Fork 353
SPICE-0024: Annotation converters
#1333
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
Conversation
1870188 to
274ba33
Compare
Annotation-based convertersAnnotation converters
97ac0af to
df576b6
Compare
2d813fe to
192ed70
Compare
stackoverflow
left a comment
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.
Look good. One question:
As annotation converters run subclass -> superclass, you may get into cases where you can't override a superclass converter right?
// some parent module
open class Super {
@SnakeCase
myProperty: String
}
// my rendered file
class Sub extends Super {
@CamelCase
myProperty: String
}Will render to my_property if I understood it correctly.
192ed70 to
2b4f066
Compare
Yeah, that's true. I wonder if this approach is worth revising. There are two main alternatives I see here:
abstract class RenameConverter extends Annotation {
abstract function rename(name: String): String
}
class SnakeCase extends RenameConverter {
// ...
}
class CamelCase extends RenameConverter {
// ...
}
output {
renderer {
annotationConverters {
[RenameConverter] = (name, annotation, value) -> Pair(annotation.rename(name), value)
}
}
}Here, only the This approach is more complex in that it's more rules to keep in mind when working with annotation converters, but it's also simpler: the user does not have to keep track of how each annotation's implementation might interact. Thoughts on which route might be preferable here? |
2b4f066 to
451d38e
Compare
|
Hmm.. I feel that it's fine that you can't override the annotations set on the superclass. I'm not actually sure if this is a problem. |
bioball
left a comment
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.
Overall, looks pretty good!
pkl-core/src/main/java/org/pkl/core/ast/member/ClassProperty.java
Outdated
Show resolved
Hide resolved
pkl-core/src/test/files/LanguageSnippetTests/input/api/jsonRenderer9.json.pkl
Outdated
Show resolved
Hide resolved
pkl-core/src/test/files/LanguageSnippetTests/output/api/jsonRenderer9.json
Outdated
Show resolved
Hide resolved
d7819e6 to
37cc4d8
Compare
BaseValueRenderer subclasses
#1413
7f434e1 to
d885f71
Compare
d885f71 to
a312617
Compare
bioball
left a comment
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.
Getting pretty close!
pkl-core/src/main/java/org/pkl/core/ast/member/ClassProperty.java
Outdated
Show resolved
Hide resolved
pkl-core/src/main/java/org/pkl/core/stdlib/AbstractRenderer.java
Outdated
Show resolved
Hide resolved
pkl-core/src/main/java/org/pkl/core/stdlib/AbstractRenderer.java
Outdated
Show resolved
Hide resolved
bioball
left a comment
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.
Approving to unblock!
But, some more suggestions (see below)
Also, we still need @Since annotations added to the new classes (like class Property in pkl:yaml).
pkl-core/src/test/files/LanguageSnippetTests/input/api/annotationConverters.pkl
Show resolved
Hide resolved
3f1ca4a to
1387ec0
Compare
1387ec0 to
d8cc4c5
Compare
stackoverflow
left a comment
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.
Looks good.
This has been banging around in my head for a while and I finally needed to get it out.
This enables defining declarative key and/or value transformations in cases where neither
Class- nor path-based converters can be applied gracefully. It is also the only way to express transforming the resulting property names inTypedobjects without applying a converter to the entire containing type, which is cumbersome at best.SPICE: apple/pkl-evolution#26
This also fixes a silly wart in the API for getting class property mirrors added in #1106.
Resolves #576