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

Switch to KotlinPoet Code Generation #28

Merged
merged 48 commits into from
Jul 1, 2022
Merged

Conversation

stabbylambda
Copy link
Contributor

@stabbylambda stabbylambda commented Jun 18, 2022

What Changed

Over the last week, @phubbard and I spent some time converting things to Kotlin Poet. The result is infinitely better than the previous string concatenation code. We were able to reduce some of the duplication between the Sync and Async versions of various hooks, the code isn't as brittle, and it's all using real type references from KSP instead of just hoping the Strings work out.

Todo:

  • SyncHook
  • The rest of the hooks
  • Delete all the godawful string manipulation code

📦 Published PR as canary version: 0.12.1-canary.28.294-SNAPSHOT

@stabbylambda stabbylambda changed the base branch from ksp to main June 22, 2022 20:12
stabbylambda and others added 4 commits June 22, 2022 13:16
Co-authored-by: Paul Hubbard <pfh@phfactor.net>
Co-authored-by: Paul Hubbard <pfh@phfactor.net>
Co-authored-by: Paul Hubbard <pfh@phfactor.net>
Co-authored-by: Paul Hubbard <pfh@phfactor.net>
@stabbylambda stabbylambda changed the title Add some KotlinPoet Code generation Switch to KotlinPoet Code Generation Jun 22, 2022
@@ -1,54 +1,62 @@
package com.intuit.hooks.plugin.codegen

import com.google.devtools.ksp.getVisibility
import com.google.devtools.ksp.symbol.*
import com.intuit.hooks.plugin.ksp.text
Copy link
Collaborator

Choose a reason for hiding this comment

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

When I was migrating from meta -> KSP in the first place, I tried to pull out all the intermediate data into pure codegen constructs, rather than a dependence on the modeling framework. This would ensure the codegen code is truly agnostic and you could potentially use the codegen module without needing the KSP part. i.e. if we want to use another processing framework (live javax.processing), or change the actual way we represent the DSL, or generate hooks via CLI.

I would like to try to continue following the strict separation of concerns. They can certainly be updated from simple String representations to whatever poet representation works best (TypeName, etc.).

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We discussed this earlier, but just putting this here for posterity. Today, we have multiple areas where we reach into the type resolver to get the information necessary to codegen the new class. When @sugarmanz looked at this, some of that was happening in the codegen, some in the validator, some in the processor. Instead what we discussed was making the whole pipeline a bit cleaner by changing it to be:

  1. process KSP -> Poet
  2. validate Poet constructs
  3. code generate from Poet constructs

That way any potential frontend (we're not switching away from KSP anytime soon) just has to convert to KotlinPoet classes.

@stabbylambda stabbylambda mentioned this pull request Jun 28, 2022
3 tasks
@sugarmanz sugarmanz merged commit 44e6650 into intuit:main Jul 1, 2022
@stabbylambda stabbylambda deleted the poet branch July 1, 2022 17:47
@sugarmanz
Copy link
Collaborator

🚀 PR was released in v0.12.1 🚀

@sugarmanz sugarmanz added the released This issue/pull request has been released. label Jul 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants