-
Notifications
You must be signed in to change notification settings - Fork 7.3k
Support using response type keeper with KSP #4526
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
base: trunk
Are you sure you want to change the base?
Conversation
| if (qualifiedName == "kotlin.Any") { | ||
| qualifiedName = "java.lang.Object" | ||
| } |
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.
Object is kept in the annotation processor, so I patched the logic for the symbol processor. Seems we have no need to do so, I can address a new PR to remove them.
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.
Yeah I left it in the annotation processor because it was harmless. I'm fine filtering them out if you want to do that in both.
|
Is KSP 2 stable? I'm pretty content being entirely outside the KSP ecosystem, and try to use it in as few places as possible. I don't want to every be forced to do a Retrofit release because a new version of Kotlin and/or KSP came out. |
|
I believe so. I didn't test the processor on KSP1 as I see KSP1 support has been dropped by https://github.com/ZacSweers/kotlin-compile-testing/releases/tag/0.10.0 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Okay I'm comfortable adding this. We actually use a KSP one internally, not the annotation processing one. I will compare to ensure this meets our needs and then merge. |
Co-authored-by: Zongle Wang <[email protected]>
Co-authored-by: Zongle Wang <[email protected]>
| @@ -0,0 +1,17 @@ | |||
| package retrofit2.keeper | |||
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.
Copyright header please
| @@ -0,0 +1,80 @@ | |||
| package retrofit2.keeper | |||
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.
Copyright header please
| w.write("# $typeName\n") | ||
| for (referencedType in referencedTypes.sorted()) { | ||
| w.write("-keep,allowoptimization,allowshrinking,allowobfuscation class $referencedType\n") | ||
| w.write("$KEEP_RULE_PREFIX $referencedType\n") | ||
| } |
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.
Instead of pulling out the constant, lets pull out this little block into a function that writes the whole file.
| "all-http-methods", | ||
| "nesting", | ||
| "kotlin-suspend", |
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.
Hmm I'm not sure we have enough tests to necessitate such a robust mechanism (as opposed to just three functions with the input source and output rules).
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.
I would probably do an enum that's like
enum class Generator {
AnnotationProcessor {
override fun validate(source: String, rules: String) { .. }
},
Ksp {
override fun validate(source: String, rules: String) { .. }
},
;
abstract fun validate(source: String, rules: String)
}and then make that enum the class-level test parameter. Then each method is basically just
@Test fun stuff() {
generator.validate(
source = """
|stuff
""".trimMargin(),
rules = """
|stuff
""".trimMargin(),
)
}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.
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.
Done. But I would still suggest using parametermized tests to reduce boilerplates.
918ea68 to
26fee81
Compare
26fee81 to
5b6c1b5
Compare
…-ksp # Conflicts: # CHANGELOG.md

CHANGELOG.md's "Unreleased" section has been updated, if applicable.