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

add optional annotations artifact with SingleIn and ForScope #725

Merged
merged 5 commits into from
Aug 8, 2023
Merged

add optional annotations artifact with SingleIn and ForScope #725

merged 5 commits into from
Aug 8, 2023

Conversation

gabrielittner
Copy link
Contributor

Closes #692

@CLAassistant
Copy link

CLAassistant commented Jul 16, 2023

CLA assistant check
All committers have signed the CLA.

@kingsleyadio
Copy link

(opinion): I think optional already has a meaning in Dagger world, which might make this naming a little confusing.
I propose changing for example, to something like annotations-extended. Other than that, love the reasoning behind including these directly in Anvil

@gabrielittner
Copy link
Contributor Author

I mostly went with the suggestion from the issue but don't really have a strong opinion on the artifact name. Happy to update the pr with something else

@vRallev
Copy link
Collaborator

vRallev commented Jul 30, 2023

It would be great to update the commit message with a better description. I'm okay with optional as name, but what I'd like to see is a DSL option that adds this dependency for you, similar to how the Gradle plugin adds the annotations artifact by default. This can be a separate PR, but this one shouldn't resolve #692.

@ZacSweers
Copy link
Collaborator

I think optional is a bad name because dagger has specialized support for an "optional" semantic via @BindsOptionalOf. I think scope-annotations or something would be better.

Also - does this need to be configurable via the DSL since it technically doesn't really have anything to do with anvil? 🤔

@vRallev
Copy link
Collaborator

vRallev commented Jul 31, 2023

If it doesn't have anything to do with Anvil, then it shouldn't be part of the repository altogether. By allowing it to be configured through the DSL we don't have to deal with Maven coordinates and it's more discoverable IMO.

@JoelWilcox
Copy link
Member

I agree that having DSL support for configuring/adding this dependency would be nice. @gabrielittner up to you if you want to take a stab at that, if not I'm happy to add it separately.

@JoelWilcox
Copy link
Member

I think optional is a bad name because dagger has specialized support for an "optional" semantic via @BindsOptionalOf. I think scope-annotations or something would be better.

Hmm this is a fair point, although I feel there aren't a lot of other name options that still accurately capture it as being a set of optional annotations while still being simple. I think something like scope-annotations could be confusing in other ways since the main annotations inherently deal with scopes too, and it could be limiting in the future if there are other optional opinionated annotations we want to add (which maybe don't directly deal with scopes).

At the moment I'd lean more towards some potential initial confusion with Dagger's concept of Optional since it should quickly become obvious it's unrelated when looking at the documentation for the artifact or directly at the included annotations. Only other name that comes to mind so far as a middle-ground would be something like annotations-extra.

These 2 optional annotations can be used to avoid having to manually
define scope annotations for each component or qualifier annotations
for each object that can clash with other. Instead all components can
use SingleIn with the same scope marker as the corresponding
MergeComponent/MergeSubcomponent/ContributesSubcomponent annotation uses.
In the same way ForScope can be used with that scope marker to qualify
bindings or provisions in a certain component.
@gabrielittner
Copy link
Contributor Author

@vRallev I've rebased and updated the commit message.

The DSL now has addOptionalAnnotations to add the dependency. It's also on VariantFilter to match the other options but if you think that's too much we can remove it from there and just directly use the main property from the extension.

@@ -94,6 +94,7 @@ subprojects {
freeCompilerArgs.add("-opt-in=kotlin.RequiresOptIn")

boolean optInExperimental = project.name != "annotations"
&& project.name != "annotations-optional"
&& project.name != "scopes"
&& project.name != "dagger-factories-only"
if (optInExperimental) {
Copy link
Member

Choose a reason for hiding this comment

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

I think the dependency substitution rules below this should also be updated to include the new artifact

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added it 6f175ec

Copy link
Member

@JoelWilcox JoelWilcox left a comment

Choose a reason for hiding this comment

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

Lgtm! Thanks for adding this

@vRallev vRallev merged commit d7adc14 into square:main Aug 8, 2023
19 checks passed
@matejdro
Copy link

Could a mention of this be added to the README? Right now this is not very discoverable.

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.

Provide SingleIn and AppScope as part of Anvil
7 participants