Support assisted injections + consolidate and clean up APIs #236

ZacSweers · 2022-10-26T07:00:28Z

Followup from #235 and more for #13

This adds support for assisted injections for common types (screen, navigator, config) and consolidates the Anvil APIs back into the same artifact for now as it really didn't end up being very useful to define it separately.

Final bit was moving remaining star sample factories to this, result was quite nice

jpetote

I have quite a few questions around CircuitSymbolProcessorProvider but i'll ask them in Slack instead.

jpetote · 2022-10-26T15:00:26Z

circuit-codegen/src/main/kotlin/com/slack/circuit/codegen/CircuitSymbolProcessorProvider.kt

        codeBlock =
          when (factoryType) {
            FactoryType.PRESENTER ->
              CodeBlock.of(
-                "%M·{·%M()·}",
+                "%M·{·%M(%L)·}",


I see that %L is assistedParams and that comes from fd which is a KSFunctionDeclarationbut what is that? And what isfd` short hand for?

fd is just shorter to write than functionDeclaration. A KSFunctionDeclaration is KSP's modeling of a kotlin function. A %L template is for literals in kotlinpoet: https://square.github.io/kotlinpoet/#l-for-literals. assistedParams is a CodeBlock of any assisted parameters we're passing on to the factory/constructor/function like (screen = screen, navigator = navigator)

jpetote · 2022-10-26T15:08:38Z

circuit/src/commonMain/kotlin/com/slack/circuit/CircuitInject.kt

-@Suppress("unused") public annotation class CircuitInject<out Screen>
-
-public annotation class CircuitScope(val scope: KClass<*>)
+public annotation class CircuitInject(


Can we add the kdoc for this now since this PR puts this in a pretty complete state.

jpetote · 2022-10-26T15:08:49Z

samples/star/src/main/kotlin/com/slack/circuit/star/home/HomePresenter.kt

-  override fun create(screen: Screen, circuitConfig: CircuitConfig): ScreenUi? {
-    if (screen is HomeScreen) {
-      return ScreenUi(homeUi())
+@CircuitInject(screen = HomeScreen::class, scope = AppScope::class)


:chefs-kiss:

jpetote · 2022-10-26T15:14:44Z

circuit-codegen/src/main/kotlin/com/slack/circuit/codegen/CircuitSymbolProcessorProvider.kt

@@ -103,21 +128,27 @@ private class CircuitSymbolProcessor(
    return emptyList()
  }

+  @OptIn(KspExperimental::class)


Since we have the file opt in at the top do we need this one here too?

Removed it as it wasn't necessary in the file level 9f36407

…rate

ZacSweers · 2022-10-26T16:50:44Z

extracted the annotations to a separate project so we keep @CircuitInject out of the core circuit artifact, since it's dagger and anvil-specific

kierse · 2022-10-26T16:44:08Z

circuit-codegen/src/main/kotlin/com/slack/circuit/codegen/CircuitSymbolProcessorProvider.kt

+}
+
+private inline fun <reified T> Resolver.loadOptionalKSType(): KSType? {
+  return getClassDeclarationByName(getKSNameFromString(T::class.java.canonicalName))


nit: there's a lint warning here about T::class.java.canonicalName being nullable

I'm not seeing anything

Weird..I'm still seeing it. canonicalName is an unannotated java method that returns a String, which supports the lint warning I'm seeing about expecting String but found String?

circuit-codegen/src/main/kotlin/com/slack/circuit/codegen/CircuitSymbolProcessorProvider.kt

iamritav · 2022-10-26T17:50:32Z

...t-codegen-annotations/src/main/kotlin/com/slack/circuit/codegen/annotations/CircuitInject.kt

+ * [Ui.Factory] classes generated for them.
+ *
+ * **Presenter**
+ * ```kotlin


what is ```kotlin for?

Same as it is in github markdown!

iamritav · 2022-10-26T17:51:37Z

...t-codegen-annotations/src/main/kotlin/com/slack/circuit/codegen/annotations/CircuitInject.kt

+@Target(AnnotationTarget.CLASS, AnnotationTarget.FUNCTION)
+public annotation class CircuitInject(
+  val screen: KClass<out Screen>,
+  val scope: KClass<*>,


why can't the scope just be of type KClass. What is the purpose of the generic here?

You can't write raw types in kotlin like you can in Java. For example, in Java it's legal to write List foo = ..., but in Kotlin you must write the generic, even if it's just a star. val foo: List = ... isn't legal syntax

samples/star/src/main/kotlin/com/slack/circuit/star/petdetail/PetDetail.kt

ZacSweers · 2022-10-26T18:33:57Z

config/detekt/detekt.yml

+    threshold: 120
+  NestedBlockDepth:
+    threshold: 6


Matches our internal config, detekt's defaults are a bit iffy. Especially with ktfmt

circuit-codegen/src/main/kotlin/com/slack/circuit/codegen/CircuitSymbolProcessorProvider.kt

...t-codegen-annotations/src/main/kotlin/com/slack/circuit/codegen/annotations/CircuitInject.kt

kierse

One suggested change related to presenter package, and that lint warning regarding use of canonicalName. Looks good otherwise

…uitSymbolProcessorProvider.kt

…codegen/annotations/CircuitInject.kt

iamritav

yay for code gen!

ZacSweers added 4 commits October 25, 2022 23:37

Support assisted injections

4719e32

Fold anvil support into main codegen artifact for now

89f3da4

Optimize object checks

a36af72

Use new code gen APIs

e0a9173

ZacSweers requested review from kierse, jpetote and iamritav October 26, 2022 07:00

jpetote reviewed Oct 26, 2022

View reviewed changes

ZacSweers added 3 commits October 26, 2022 09:24

Remove file optin

9f36407

Doc CircuitInject

e75b6c8

Extract circuit-codegen-annotations module to keep CircuitInject sepa…

f2d75da

…rate

kierse reviewed Oct 26, 2022

View reviewed changes

iamritav reviewed Oct 26, 2022

View reviewed changes

samples/star/src/main/kotlin/com/slack/circuit/star/petdetail/PetDetail.kt Outdated Show resolved Hide resolved

ZacSweers added 8 commits October 26, 2022 10:59

Fix imports

9eb69c5

Baseline classpaths

3e4a17b

Extract computeFactoryData() function

0a038da

Doc assistedParameters

a55c5d6

Make detekt happy

5e3082f

Update detekt config

90d5747

Refactor assistedParameters() block a bit

33749d8

Formatting

b3f201c

ZacSweers commented Oct 26, 2022

View reviewed changes

kierse reviewed Oct 26, 2022

View reviewed changes

circuit-codegen/src/main/kotlin/com/slack/circuit/codegen/CircuitSymbolProcessorProvider.kt Outdated Show resolved Hide resolved

kierse reviewed Oct 26, 2022

View reviewed changes

...t-codegen-annotations/src/main/kotlin/com/slack/circuit/codegen/annotations/CircuitInject.kt Outdated Show resolved Hide resolved

kierse reviewed Oct 26, 2022

View reviewed changes

ZacSweers added 2 commits October 26, 2022 16:59

Update circuit-codegen/src/main/kotlin/com/slack/circuit/codegen/Circ…

3662178

…uitSymbolProcessorProvider.kt

Update circuit-codegen-annotations/src/main/kotlin/com/slack/circuit/…

1f4733a

…codegen/annotations/CircuitInject.kt

ZacSweers requested review from kierse, jpetote and iamritav October 26, 2022 21:03

iamritav approved these changes Oct 26, 2022

View reviewed changes

jpetote approved these changes Oct 26, 2022

View reviewed changes

ZacSweers merged commit 15010e7 into main Oct 27, 2022

ZacSweers deleted the z/assisted branch October 27, 2022 00:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support assisted injections + consolidate and clean up APIs #236

Support assisted injections + consolidate and clean up APIs #236

ZacSweers commented Oct 26, 2022

jpetote left a comment

jpetote Oct 26, 2022

ZacSweers Oct 26, 2022

jpetote Oct 26, 2022

ZacSweers Oct 26, 2022

jpetote Oct 26, 2022

jpetote Oct 26, 2022

ZacSweers Oct 26, 2022

ZacSweers commented Oct 26, 2022

kierse Oct 26, 2022

ZacSweers Oct 26, 2022

kierse Oct 26, 2022

iamritav Oct 26, 2022

ZacSweers Oct 26, 2022

iamritav Oct 26, 2022

ZacSweers Oct 26, 2022

ZacSweers Oct 26, 2022

kierse left a comment

iamritav left a comment

Support assisted injections + consolidate and clean up APIs #236

Support assisted injections + consolidate and clean up APIs #236

Conversation

ZacSweers commented Oct 26, 2022

jpetote left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZacSweers commented Oct 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kierse left a comment

Choose a reason for hiding this comment

iamritav left a comment

Choose a reason for hiding this comment