-
Notifications
You must be signed in to change notification settings - Fork 88
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
Migrate ProvidesMethodFactoryCodeGen to KSP #806
Migrate ProvidesMethodFactoryCodeGen to KSP #806
Conversation
@JoelWilcox @RBusarow this is ready now 👍. Last major one of the simple generators. |
@@ -37,6 +37,7 @@ dependencies { | |||
|
|||
compileOnly libs.auto.service.annotations | |||
compileOnly libs.kotlin.compiler | |||
compileOnly libs.ksp.compilerPlugin |
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 is for internal API access, see comment below
val className = clazz.toClassName() | ||
val containingFile = clazz.containingFile!! | ||
// TODO we need a public API for this in KSP | ||
// https://github.com/google/ksp/issues/1621 |
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.
They're open to this, so I'll send a PR to them soon. In the meantime, we do this internal access in #713 anyway so it's not necessarily new
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.
Could you add a commit just for the file/class rename? To help highlight what is changed/unchanged, and potentially preserve some of the history depending on if we squash later
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'd have to restart the PR from scratch and I'm not sure if you can keep that history given the squash and merge?
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.
Not sure what you mean by starting from scratch, you should be able to just squash your changes, commit the file rename, and then commit the rest. And then with just those two commits there's no need to squash and merge 😁 . Next time though! No worries for this one
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.
ahh I thought you meant while preserving the other commits (which I did try to keep somewhat focused). 👍
// KSP always resolves the inferred return type | ||
assumeFalse(mode is AnvilCompilationMode.Ksp) |
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.
Can you add a new complimentary test for KSP that verifies this use-case passes? Or an if/else on the asserts here. Sounds like expectations change here once KSP is enabled so we should verify that and make sure to catch if there's ever a regression
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.
Nice! Thanks for updating
…tegration * main: Update ktlintPlugin to v12.0.3 Migrate ProvidesMethodFactoryCodeGen to support KSP (#806) Copy the root `gradle` directory during benchmark project generation Update junit5 monorepo to v5.10.1 # Conflicts: # gradle/libs.versions.toml
Ref #751.