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 messages to replaced explicit bindings #158

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

Conversation

TWiStErRob
Copy link
Contributor

@TWiStErRob TWiStErRob commented Aug 3, 2019

Fix 3 TODOs and cover them with tests. Note: I missed mapOfLazyKey deliberately, see #159.


Here's the Dagger output for these issues:

integration-tests\src\main\java\com\example\MultibindingSetConflict.java:13:
error: [Dagger/DuplicateBindings] java.util.Set<java.lang.String> has incompatible bindings or declarations:
interface MultibindingSetConflict {
^
      Set bindings and declarations:
          @Provides @dagger.multibindings.IntoSet String com.example.MultibindingSetConflict.Module1.one()
          @Provides @dagger.multibindings.ElementsIntoSet Set<String> com.example.MultibindingSetConflict.Module1.three()
          @Provides @dagger.multibindings.IntoSet String com.example.MultibindingSetConflict.Module1.two()
      Unique bindings and declarations:
          @Provides Set<String> com.example.MultibindingSetConflict.Module1.explicit()
      java.util.Set<java.lang.String> is provided at
          com.example.MultibindingSetConflict.values()
1 error

integration-tests\src\main\java\com\example\MultibindingMapConflict.java:12:
error: [Dagger/DuplicateBindings] java.util.Map<java.lang.Integer,java.lang.String> has incompatible bindings or declarations:
interface MultibindingMapConflict {
^
      Map bindings and declarations:
          @Provides @dagger.multibindings.IntoMap @dagger.multibindings.IntKey(1) String com.example.MultibindingMapConflict.Module1.one()
          @Provides @dagger.multibindings.IntoMap @dagger.multibindings.IntKey(2) String com.example.MultibindingMapConflict.Module1.two()
      Unique bindings and declarations:
          @Provides Map<Integer,String> com.example.MultibindingMapConflict.Module1.explicit()
      java.util.Map<java.lang.Integer,java.lang.String> is provided at
          com.example.MultibindingMapConflict.values()
1 error

integration-tests\src\main\java\com\example\MultibindingMapProviderConflict.java:13:
error: [Dagger/DuplicateBindings] java.util.Map<java.lang.Integer,javax.inject.Provider<java.lang.String>> has incompatible bindings or declarations:
interface MultibindingMapProviderConflict {
^
      Map bindings and declarations:
          @Provides @dagger.multibindings.IntoMap @dagger.multibindings.IntKey(1) String com.example.MultibindingMapProviderConflict.Module1.one()
      Unique bindings and declarations:
          @Provides Map<Integer,Provider<String>> com.example.MultibindingMapProviderConflict.Module1.explicit()
      java.util.Map<java.lang.Integer,javax.inject.Provider<java.lang.String>> is provided at
          com.example.MultibindingMapProviderConflict.values()
1 error

builder.append(key).append(" has incompatible bindings or declarations:\n");
builder.append(" Set bindings and declarations:\n");
for (Binding elementBinding : setBindings.elementBindings) {
builder.append(" @IntoSet ").append(elementBinding).append("\n");
Copy link
Owner

Choose a reason for hiding this comment

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

Scope shouldn't know anything about Dagger. Can we change these messages in a way to provide this information without encoding the implementation details of how Dagger contributes bindings into a set? Should the binding itself report this information in its toString() perhaps?

Copy link
Contributor Author

@TWiStErRob TWiStErRob Aug 5, 2019

Choose a reason for hiding this comment

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

The Binding doesn't know it came from @IntoSet/@ElementsIntoSet/@IntoMap:
image
unless we look into method.declaredAnnotations.

I removed dagger.** class references and the dagger error code and fixed sentence comments in force push

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at Dagger output that's what's happening there too, happy to open a followup changing the toString of /(un)?linked(provides|binds)binding/ to include the full signature including annotations and parameters.


Did a quick test with:

  public String toString() {
    return "@Provides["
+        + Arrays.toString(method.getDeclaredAnnotations())
        + method.getDeclaringClass().getName() + '.' + method.getName() + "(…)]";
  }
  Set bindings and declarations:
    [single  ] @Binds[[@dagger.Binds(), @dagger.multibindings.IntoSet()]com.example.MultibindingSetConflict$Module1.three(…)]
    [single  ] @Provides[[@dagger.Provides(), @dagger.multibindings.IntoSet()]com.example.MultibindingSetConflict$Module1.one(…)]
    [multiple] @Provides[[@dagger.Provides(), @dagger.multibindings.ElementsIntoSet()]com.example.MultibindingSetConflict$Module1.two(…)]
    [multiple] @Binds[[@dagger.Binds(), @dagger.multibindings.ElementsIntoSet()]com.example.MultibindingSetConflict$Module1.four(…)]

I think I would need to remove the Binding's own annotation, and the noisy bits like () and known package names like dagger/dagger.mulitbindings/javax.inject to make it useful.

Copy link
Owner

Choose a reason for hiding this comment

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

You don't need to pull it from the method. We can supply an enum value or string at the time of binding creation which helps format this message.

@TWiStErRob TWiStErRob force-pushed the replaced_explicit_bindings branch from f9c5748 to c89290e Compare August 5, 2019 22:43
@TWiStErRob TWiStErRob force-pushed the replaced_explicit_bindings branch from c89290e to db6b03c Compare August 5, 2019 22:51
"java.util.Set<java.lang.String> has incompatible bindings or declarations:\n"
+ " Set bindings and declarations:\n"
+ " [single ] @Provides[com.example.MultibindingSetConflict$Module1.one(…)]\n"
+ " [multiple] @Provides[com.example.MultibindingSetConflict$Module1.two(…)]\n"
Copy link
Owner

Choose a reason for hiding this comment

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

What about "instance" and "elements"? We can re-use terminology like "elements" which is in the API of Scope, I just don't want explicitly mechanisms of Dagger like @ElementsIntoSet (unless we sneak them into the Binding's toString(), then we can have as much Dagger-specific things as we like).

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.

2 participants