Skip to content

Conversation

nbauernfeind
Copy link
Member

This fixes #29.

I rewrote the ScalaMultibinder to work much more like Guice's Multibinder.

  • It's now a trait with a hidden implementation RealScalaMultibinder.
  • RealScalaMultibinder is a module that tricks Guice into only installing it once (by pretending to be the scala set key).
  • I also filter all of the methods through three paths: TypeLiteral, TypeLiteral + Class[Annotation], and TypeLiteral + Annotation.

You may want to pull down this code and look at it in your favorite editor. I put Scala's specific methods at the top of ScalaMultibinder so that people may find the most useful things at the top of the file; however now the pull-request patch is slightly difficult to follow.

@tsuckow
Copy link
Member

tsuckow commented Oct 4, 2014

I'll take a look at this when I can. I want to check the java compatibility, not sure how important that is. I'm currently moving this weekend but should be able to get to it by Wednesday. Kick me if I don't.

@nbauernfeind nbauernfeind force-pushed the Multibinder branch 9 times, most recently from a954561 to 53d8847 Compare October 5, 2014 15:20
@nbauernfeind
Copy link
Member Author

Hey Thomas,

I ended up refactoring and more carefully rewriting portions of these two initial pull requests. You'll probably want to look at #32 first since it is the code this is common in the other three pull requests (this one #30, #31, and #33). If I could get some feedback on what you'd like to see changed that'd be great =).

Thanks,
Nate

@tsuckow
Copy link
Member

tsuckow commented Oct 13, 2014

Will do. I started looking through the changes on Friday but got busy again
this weekend.
On Oct 12, 2014 9:11 PM, "Nate Bauernfeind" notifications@github.com
wrote:

Hey Thomas,

I ended up refactoring and more carefully rewriting portions of these two
initial pull requests. You'll probably want to look at #32
#32 first since it is the
code this is common in the other three pull requests (this one #30
#30, #31
#31, and #33
#33). If I could get some
feedback on what you'd like to see changed that'd be great =).

Thanks,
Nate


Reply to this email directly or view it on GitHub
#30 (comment).

@tsuckow
Copy link
Member

tsuckow commented Oct 14, 2014

I am making my way through the commits. So far, so good.

Though I have noticed some of the commit messages don't mention everything that changed. Such as sneaking a scala version upgrade in with updating Guice. I'll let it slide this time. ;)

@nbauernfeind
Copy link
Member Author

Ahh, oops. I had originally had it broken down much more, but then I had refactored some things and the commits quickly became spaghetti like. Additionally github also doesn't handle commits in out-of-date order very well. It orders them by date instead of tree order (which can be very confusing).

@tsuckow
Copy link
Member

tsuckow commented Oct 14, 2014

I honestly didn't really look at them on github. I have a fetch rule that
will bring over pull requests and then I look at them in gitk
On Oct 14, 2014 4:38 AM, "Nate Bauernfeind" notifications@github.com
wrote:

Ahh, oops. I had originally had it broken down much more, but then I had
refactored some things and the commits quickly became spaghetti like.
Additionally github also doesn't handle commits in out-of-date order very
well. It orders them by date instead of tree order (which can be very
confusing).


Reply to this email directly or view it on GitHub
#30 (comment).

@rocketraman
Copy link

As noted in this stackoverflow question, the code in this pull request worked when I experienced issue #29. Thanks!

@tsuckow tsuckow added this to the 4.0.0-beta5 milestone Oct 25, 2014
@tsuckow tsuckow added the bug label Oct 25, 2014
@tsuckow tsuckow merged commit 452f515 into codingwell:develop Oct 25, 2014
@nbauernfeind nbauernfeind deleted the Multibinder branch October 25, 2014 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ScalaMultibinder doesn't allow multiple modules to contribute, as Guice Multibinder does

3 participants