Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
@ting-yuan @jaschdoc I rebased with the correct email for CLA, let me know if there's anything I need! |
jaschdoc
left a comment
jaschdoc
left a comment
There was a problem hiding this comment.
Hi @jonamireh, thank you for the PR! Looks good overall. I left some feedback to improve readability and tests.
| (annotationApplication.psi as? KtAnnotationEntry)?.typeReference?.let { | ||
| KSTypeReferenceImpl.getCached(it, this@KSAnnotationResolvedImpl) | ||
| } ?: annotationApplication.psi?.resolveCall()?.signature?.returnType?.let { | ||
| KSTypeReferenceResolvedImpl.getCached(it, parent = this@KSAnnotationResolvedImpl) | ||
| } ?: KSTypeReferenceResolvedImpl.getCached( |
There was a problem hiding this comment.
Could you either name things a bit more or explain in comments what happens here?
| ) | ||
| } | ||
|
|
||
| override fun toResult(): List<String> = results |
There was a problem hiding this comment.
This should be sorted to ensure results are stable across changes to the resolver. You might also need to update the expected output in annotationTypeArguments.kt.
| private fun renderAnnotation(label: String, annotation: KSAnnotation) { | ||
| val typeArguments = annotation.annotationType.element!!.typeArguments | ||
| results.add("$label.annotationType: ${annotation.annotationType}") | ||
| results.add("$label.typeArgCount: ${typeArguments.size}") | ||
| results.add( | ||
| "$label.typeArgs: ${ | ||
| typeArguments.joinToString { argument -> | ||
| argument.type!!.resolve().declaration.qualifiedName!!.asString() | ||
| } | ||
| }" | ||
| ) | ||
| } |
There was a problem hiding this comment.
Nit: Could you split rendering and adding to list into separate functions or just add the list of strings to the result at the call site? Example:
renderAnnotation(propertyName, annotation).forEach { results.add(it) }| override fun process(resolver: Resolver): List<KSAnnotated> { | ||
| listOf("target", "nestedTarget").forEach { propertyName -> | ||
| val property = resolver.getPropertyDeclarationByName(resolver.getKSNameFromString(propertyName), true)!! | ||
| val annotation = property.annotations.single { it.shortName.asString() == "MapConvert" } | ||
| renderAnnotation(propertyName, annotation) | ||
| } |
There was a problem hiding this comment.
Ideally, you should obtain these properties by calling resolver.getSymbolsWithAnnotation, so if the test is ever updated, the processor will catch the change.
There was a problem hiding this comment.
You could also change the targets to classes instead of val-bound unit values.
| val containerProperty = | ||
| resolver.getPropertyDeclarationByName(resolver.getKSNameFromString("containerTarget"), true)!! | ||
| val containerAnnotation = containerProperty.annotations.single { it.shortName.asString() == "Container" } | ||
| val nestedAnnotation = containerAnnotation.arguments.single().value as KSAnnotation | ||
| renderAnnotation("containerTarget.nested", nestedAnnotation) |
There was a problem hiding this comment.
The same thing applies here. You should obtain containerTarget by calling resolver.getSymbolsWithAnnotation("Container")
|
Thanks for the feedback! Addressed them all 🙂 |
2 participants
Summary
This PR fixes #2622 to allow type arguments to be read from parameterized annotations. Additionally, this PR also allows them to be read when the parameterized annotation is an argument to another annotation
Test Plan
Compiler tests were added for all of these cases
🤖 Co-authored with Codex