-
Notifications
You must be signed in to change notification settings - Fork 2
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
KSP Migration #35
KSP Migration #35
Conversation
I'm realizing this repo sprung up before the |
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.
Pass 1/x (mostly focused on build setup this pass)
This was done to solve issues with stubs across multiple modules. KSP was not properly carrying annotation arguments across the module boundary (that is, even though we could see annotations on the stub class, @Exercise.params was artificially empty).
Most recent change is a workaround for google/ksp#356 |
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.
Pass 2/x
50 / 91 files viewed
Is the plan to eventually support it again, or are we abandoning AndroidX result support? |
The plan is definitely to support it again. It just didn't feel right to keep it considering that it was both some of the only generated code without tests, and also not generating code compatible with the 1.x releases of androidx result passing. |
Im not sure who else this would be useful for (highly likely just for myself) but I found https://github.com/google/ksp/blob/master/docs/why-ksp.md very informative for a high level background on KSPs. |
Probably learned more from this PR than I am able to offer in advice or criticism, and I don't want to hold it back any longer. Maybe discuss more about your findings on working with KSPs at the Kotlin team meeting today? |
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.
Pass 3/x
I know my review is taking quite a while. Really wanting to understand this stuff. It is super cool too! Let me know if there is any urgency to getting this in and I'll prioritize and/or review with less detail/questions.
A lot of the slow down of my review is trying to learn it as I review it. So, I also apologize for asking a lot of questions that purely stem from me having zero experience with 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.
This is really phenomenal! 💯
Thanks for taking the time to help me understand it better. ❤️
@@ -0,0 +1,5 @@ | |||
plugins { | |||
kotlin("jvm") | |||
// Not real code, so not worrying about coverage or lints. |
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 you really need this comment. 🤷
Major Breaking Changes
kapt
support in favor ofksp
supportOther Important Changes
XyzParams
class is now generated in the stub module, accepting anyActivity
in its constructor. This is a workaround for a bug in KSP, and should be revertible pre-1.0 for stronger type safety.build.gradle.kts
and abuildSrc
moduleCloses #26.