-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Module overrides + multibindings = pain #174
Comments
I'm not opposed - we're early enough to break things… the principle at Truthfully, this early, I'm ok for either - and if we want stronger, |
My 2 cents: I think override should replace, not expand multibindings, both to accomodate the test replacement use case, and for consistency with single bindings. |
I'm currently using additive feature of multibindings to allow users to add -A On Fri, Mar 8, 2013 at 5:35 PM, Andy Dennie notifications@github.comwrote:
|
@swankjesse @jclouds just to clarify my earlier comment, I acknowledge the use case for additive bindings, but "override" doesn't seem to me like the right vehicle for it. |
I'm actually opposed to this (despite being the person the current impl bit pretty hard). I think our failure is that we are using Do we even know if set bindings and child graphs behave properly? class Module1 {
@Provides(type = SET) String a() { return "a"; }
@Provides(type = SET) String b() { return "b"; }
}
class Module2 {
@Provides(type = SET) String c() { return "c"; }
} What does a graph with module 1 plussed to module 2 get me? |
@JakeWharton currently that gives you an error, which is the right behavior in my opinion. |
I follow you guys because this is really interesting. I agree with the override behavior of just replacing stuff (where this means replacing just dependencies or branches depending on the implementation). The adding behavior, for testing or not, is already in the Module composition. New module, new branch. Raising an exception if there are conflicts like above. IHMO (as always) and not knowing much about the current implementation :) Thanks! |
Where did we land with this? |
jeez this one's a tough call. I need to play with SET_VALUES in 1.1 more as this was the only place I needed overrides=true on set bindings before. |
Unless we find a really compelling reason to change, my vote is to do nothing. It behaves as expected when you think about it. It just surprised me when I ran into it. |
Currently the overriding module replaces instead of adds to the set of multibindings. We should make a behavior-breaking-change and combine 'em all.
The text was updated successfully, but these errors were encountered: