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

Implement KSP support for ContributesMultibinding #740

Merged
merged 7 commits into from
Nov 14, 2023

Conversation

ZacSweers
Copy link
Collaborator

Fairly straightforward, but added support for a SimpleSymbolProcessor for use in tests.

@ZacSweers ZacSweers force-pushed the z/ksp/contributesMultibinding branch from 2e21ec6 to ed5286c Compare August 2, 2023 05:52
}
is AnvilCompilationMode.Ksp -> {
val processor = simpleSymbolProcessor { resolver ->
resolver.getSymbolsWithAnnotation(MergeComponent::class.qualifiedName!!)
Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason to use qualifiedName over canonicalName or vice versa? Just asking since I noticed you used getSymbolsWithAnnotation(ContributesMultibinding::class.java.canonicalName) in the code gen

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No functional difference, one is just java reflection and the other is kotlin reflection. Thinking we could maybe standardize toward the latter to make it more KMP-friendly in the future?

Copy link
Member

Choose a reason for hiding this comment

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

👍 Sounds good to me


val dependencies = Dependencies(aggregating = false, sources = emptyArray())
val file = env.codeGenerator.createNewFile(dependencies, packageName, "$fileName.kt")
// Don't use writeTo(file) because that tries to handle directories under the hood
Copy link
Member

Choose a reason for hiding this comment

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

What ends up happening if writeTo was used here? Would it result in an obvious failure or something more cryptic/delayed? Thinking about if this is a footgun that could bite us elsewhere in the future

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Under the hood Kotlinpoet tries to make directories for package segments, but KSP does that for us already so we don't want to double-nest. It wouldn't obviously fail, it would just create a duplicate bested package structure. I don't think it's something we need to really guard against as this is the only place we do this custom.

@ZacSweers
Copy link
Collaborator Author

idk what to do about that windows job

@ZacSweers ZacSweers mentioned this pull request Sep 27, 2023
15 tasks
@JoelWilcox JoelWilcox merged commit 0cea83e into square:main Nov 14, 2023
19 checks passed
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.

None yet

2 participants