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

ContributeSubComponent: Support returning Super Type #83

Conversation

esafirm
Copy link

@esafirm esafirm commented Nov 27, 2024

Note

The original PR is in here: square#1070
But since we're using the KSP fork, we also create this PR.

This PR enables the ContributesSubcomponent.Factory to return the component super type.
The capability of returning component super type is available in @MergeComponent and is also supported in Dagger.

An example of this can be super useful is when we have a base factory.

interface BaseSubcomponent {
  interface Factory {
    fun createFactory(): BaseSubcomponent
  }
}

@ContributesSubcomponent.Factory 
interface Factory : BaseSubcomponent.Factory

esafirm and others added 4 commits November 27, 2024 12:32
This deprecates `ClassReference.functions` and `ClassReference.properties` in favor of two new properties each.

Changes:

|     old, now deprecated     | new                                                                               |
|:---------------------------:|-----------------------------------------------------------------------------------|
| `ClassReference.functions`  | `ClassReference.memberFunctions` </br> `ClassReference.declaredMemberFunctions`   |
| `ClassReference.properties` | `ClassReference.memberProperties` </br> `ClassReference.declaredMemberProperties` |
Copy link
Owner

@ZacSweers ZacSweers left a comment

Choose a reason for hiding this comment

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

Thanks! Few small comments

CHANGELOG.md Outdated
Comment on lines 7 to 8
- `ClassReference.functions` has been deprecated in favor of `ClassReference.memberFunctions` and `ClassReference.declaredMemberFunctions`
- `ClassReference.properties` has been deprecated in favor of `ClassReference.memberProperties` and `ClassReference.declaredMemberProperties`
Copy link
Owner

Choose a reason for hiding this comment

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

Can remove these since I'm removing the K1 support anyway

Comment on lines 414 to 416
val isReturningSuperType = returnType != null && contribution.clazz.superTypes.any {
it.resolve().resolveKSClassDeclaration() == returnType
}
Copy link
Owner

Choose a reason for hiding this comment

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

There is a Resolver API for checking if one type is a subtype of another. I don't remember the name exactly but we should use that one

Copy link
Author

Choose a reason for hiding this comment

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

I found KSType.isAssignableFrom

So something like this:

returnTypeToCheck.isAssignableFrom(contribution.clazz.asType(emptyList()))

@ZacSweers
Copy link
Owner

@esafirm do you still plan to revisit this?

@esafirm
Copy link
Author

esafirm commented Dec 11, 2024

@ZacSweers yes, although I'm not sure about this part.

This fix needs the ClassReference.memberFunctions. Should I remove the ClassReference.memberProperties only?

@ZacSweers
Copy link
Owner

Yes no need to worry about supporting K1. You can disable K1/embedded modes for new tests that target this, I'm going to delete them

@esafirm
Copy link
Author

esafirm commented Dec 16, 2024

@ZacSweers I remove the memberProperties and changes in ClassReferenceTest.

Not really sure about the CI checks as API check and Ktlint are green on my local 🤔

Let me know if you need anything else.

@ZacSweers
Copy link
Owner

CI appears to be failing on much more than just formatting and API dump?

@esafirm
Copy link
Author

esafirm commented Dec 27, 2024

@ZacSweers took a closer look. Now all warnings are resolved.

@ZacSweers
Copy link
Owner

It does not appear that way

@esafirm
Copy link
Author

esafirm commented Dec 30, 2024

@ZacSweers Let's try once again. Just a heads up, I've introduced changes in ClassReferenceTest again to fix the CI.

Comment on lines +12 to +13
assert(baseComponent.description() == UserDescriptionModule.provideDescription())
assert(userComponent.username() == UserDescriptionModule.provideName())
Copy link
Owner

Choose a reason for hiding this comment

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

I'll fix this after merging but in the future please use assertThat test helpers like the rest of the tests

Copy link
Author

Choose a reason for hiding this comment

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

🙏

@ZacSweers ZacSweers merged commit 0f07e65 into ZacSweers:main Jan 1, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants