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

Allow contributing bindings with generic supertypes #726

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

matejdro
Copy link

@matejdro matejdro commented Jul 21, 2023

Instead of giving up, whenever user attempts to @Contributes(Multi)Binding on a generic parent interface, it will just bind to a variant of the parent interface with generic parameters filled as stars.

It does not cover all use cases, but it covers more use cases than the old solution. And user can still use regular Module to achieve the rest of the use cases (like before), so this seems like a win win in my book.

This fixes #694.

Anvil will always contribute bindings with all generic types filled as a star
@CLAassistant
Copy link

CLAassistant commented Jul 21, 2023

CLA assistant check
All committers have signed the CLA.

@gabrielittner
Copy link
Contributor

For multibinding using * definitely makes sense.

However I'm wondering whether for regular bindings using the actual type parameter would make more sense. For example if I bind SQLDelight's ColumnAdpater, I'd probably would want the bound type for class InstantColumnAdapter : ColumnAdapter<String, Instant> to be ColumnAdapter<String, Instant>.

@vRallev
Copy link
Collaborator

vRallev commented Jul 30, 2023

This was debated over and over again and there are other tickets discussing this. We didn't want to support this, because it adds inconsistency with boundType. Imagine this example:

@ContributesBinding(AppScope::class, boundType = CharSequence::class)
@ContributesMultbinding(AppScope::class)
object MyClass : List<String>, CharSequence { .. }

Which super type should be used is unclear, you'd need to use boundType similar to ContributesBinding in the sample. But boundType doesn't allow define the generic type.

You can always support your use case with a custom code generator.

@matejdro
Copy link
Author

matejdro commented Jul 31, 2023

For the @ContributesBinding issue, what if we just take the actual parent type instead of *? So in your example @ContributesBinding would bind into ColumnAdapter<String, Instant>. However, @ContributesMultibinding would still bind star bindings, so it would bind ColumnAdapter<*, *>.

As for theboundType issue, what if you just need to specify the type and type parameter are inferred from actual paremters? So, in your case, you would enter @ContributesMultbinding(AppScope::class, boundType = List::class) and Anvil would know that you meant List<String>, since MyClass extends it.

@matejdro
Copy link
Author

Added extra commits with above issues addressed.

As part of this I also had to fix a bug in isTypeParameter function, where this function would falsely return false if parent type had either out or in keywords (d7f0917).

@blakelee
Copy link

This has come up a few times on my team and we'd really like to have this functionality. Even if it doesn't fix every issue, it would be nice to at least do something, even if it's only generic supertypes for now.

@ramzyabushaaban
Copy link

Any solution for this use case?

@matejdro
Copy link
Author

I can look into solving conflicts and getting this into mergeable state if I get some feedback from Anvil team that this is actually something they want to get merged. They have been pretty silent so far.

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.

Allow type parameters binding, with star type added
6 participants