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.

Sign up for GitHub

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

Jump to bottom

KSP Migration #35

Merged
merged 14 commits into from
Mar 25, 2021
Merged

KSP Migration #35

merged 14 commits into from
Mar 25, 2021

Conversation

cedrickcooke
Copy link
Collaborator

@cedrickcooke cedrickcooke commented Mar 19, 2021 •
edited
Loading

Major Breaking Changes

Other Important Changes

  • For stubs, XyzParams class is now generated in the stub module, accepting any Activity in its constructor. This is a workaround for a bug in KSP, and should be revertible pre-1.0 for stronger type safety.
  • Use build.gradle.kts and a buildSrc module
  • Fully separate input parsing from output generation
  • Export stubs to their own module

Closes #26.

@cedrickcooke cedrickcooke requested review from a team, twyatt and sdonn3 March 19, 2021 17:47
@cedrickcooke
Copy link
Collaborator Author

I'm realizing this repo sprung up before the master -> main transition, so some of the copied CI is set up against the wrong branch. After this PR is merged I'll make the migration to main for the repo.

twyatt
twyatt reviewed Mar 20, 2021
View reviewed changes
Copy link
Member

@twyatt twyatt left a 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)

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/publish.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/publish.yml Outdated Show resolved Hide resolved
.github/workflows/snapshot.yml Outdated Show resolved Hide resolved
.github/workflows/publish.yml Outdated Show resolved Hide resolved
build.gradle.kts Outdated Show resolved Hide resolved
build.gradle.kts Outdated Show resolved Hide resolved
build.gradle.kts Outdated Show resolved Hide resolved
stubs/.gitignore Outdated Show resolved Hide resolved
@twyatt twyatt self-requested a review March 20, 2021 06:51
@cedrickcooke
More fixes in ci and gradle scripts
092ded2
@cedrickcooke
Generate params class with @AsStub instead of @FromStub
276de69 
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).
@cedrickcooke
Copy link
Collaborator Author

Most recent change is a workaround for google/ksp#356

twyatt
twyatt reviewed Mar 23, 2021
View reviewed changes
Copy link
Member

@twyatt twyatt left a comment •
edited
Loading

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

compile/build.gradle.kts Outdated Show resolved Hide resolved
@twyatt twyatt self-requested a review March 23, 2021 07:03
@twyatt twyatt mentioned this pull request Mar 23, 2021
Merged
@twyatt
Copy link
Member

twyatt commented Mar 23, 2021

Removes the experimental support for AndroidX result passing (tracked in #20 anyways; we supported an alpha version, and the API changed out from under us).

Is the plan to eventually support it again, or are we abandoning AndroidX result support?

@cedrickcooke
Copy link
Collaborator Author

Removes the experimental support for AndroidX result passing (tracked in #20 anyways; we supported an alpha version, and the API changed out from under us).

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.

@sdonn3
Copy link

sdonn3 commented Mar 24, 2021

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.

@sdonn3
Copy link

sdonn3 commented Mar 24, 2021

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?

twyatt
twyatt reviewed Mar 25, 2021
View reviewed changes
Copy link
Member

@twyatt twyatt left a comment •
edited
Loading

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.

compile/src/main/kotlin/ExerciseProcessor.kt Show resolved Hide resolved
compile/src/main/kotlin/Logging.kt Show resolved Hide resolved
compile/src/main/kotlin/read/Dependencies.kt Outdated Show resolved Hide resolved
compile/src/main/kotlin/read/Parameters.kt Show resolved Hide resolved
compile/src/main/kotlin/read/Parameters.kt Outdated Show resolved Hide resolved
compile/src/main/kotlin/read/Parameters.kt Outdated Show resolved Hide resolved
compile/src/main/kotlin/read/Parameters.kt Outdated Show resolved Hide resolved
@twyatt twyatt self-requested a review March 25, 2021 05:57
twyatt
twyatt approved these changes Mar 25, 2021
View reviewed changes
Copy link
Member

@twyatt twyatt left a 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. ❤️

stubs/build.gradle.kts
@@ -0,0 +1,5 @@
plugins {
kotlin("jvm")
// Not real code, so not worrying about coverage or lints.
Copy link
Member

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. 🤷

@cedrickcooke cedrickcooke merged commit 1255e06 into master Mar 25, 2021
@cedrickcooke cedrickcooke deleted the cedrickc/ksp-migration branch March 25, 2021 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@twyatt twyatt twyatt approved these changes

@sdonn3 sdonn3 sdonn3 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Explore Kotlin Symbol Processing
3 participants
@cedrickcooke @twyatt @sdonn3