-
Notifications
You must be signed in to change notification settings - Fork 44
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
base: master
Are you sure you want to change the base?
Add messages to replaced explicit bindings #158
Conversation
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"); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
:
unless we look into method.declaredAnnotations
.
I removed dagger.**
class references and the dagger error code and fixed sentence comments in force push
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
f9c5748
to
c89290e
Compare
c89290e
to
db6b03c
Compare
"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" |
There was a problem hiding this comment.
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).
Fix 3 TODOs and cover them with tests. Note: I missed
mapOfLazyKey
deliberately, see #159.Here's the Dagger output for these issues: