make the injector take into account custom injectables #42

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

@dpick

I started building an example app backed by datamapper and quickly realized that custom injectables weren't being used. This seems to fix the issue but I wasn't sure of the best way to write a test for it. Any thoughts?

@dpick

Yeah sorry, I vendored raptor for the example app and made the change there. Then obviously was not paying close enough attention when I made the change in the actual repo.

I've updated the branch.

@tcrayford
Collaborator

I'm pretty sure you can write a test for it. Seeing as this bug was in the router, maybe write a more integrated test that checks the router can inject custom injectables into things?

Also, can't you just delete the line injector = injector.add_request(request)? The line above it already adds the request to the injector.

@dpick

You're right that 2nd line isn't necessary.

I took a stab at writing a test for it, but it ended up really really ugly.

@tcrayford
Collaborator

This is good to merge, and has been for ages. I think we want an integration test like that for custom injectables anyway (though it is somewhat ugly).

@tcrayford
Collaborator

dpick: care to rebase master into this so we don't have a horrible history?

@dpick

I rebase master in and squashed my commits into one, but the test fails now. I'll look into why later tonight and push a fix.

@zombor

Any update on this? I ran into this problem, and commenting the second line fixed the problem.

@tcrayford
Collaborator

@dpick: did you get around to fixing your tests? If not, I'll probably be pushing a fix for this soonish.

@dpick

@tcrayford Go for it, things got a little crazy at work and I haven't had a chance to look at it.

@garybernhardt

This wasn't explicitly merged, but it does seem to have been fixed.

@garybernhardt

Wait, maybe not. Just saw @zombor's comment.

@garybernhardt garybernhardt reopened this Jun 11, 2012
@garybernhardt

No idea why that method was constructing two injectors. There's now only one line matching /Injector./ in lib, which is as it should be.

I extended the app integration tests to catch this. That might be Raptor's first concession to "things are broken and we must do evil to unbreak them quickly". ;)

@dpick
@garybernhardt

Awesome. Feel free to open issues for discussion! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment