Adding support for constrained open generics to DI #34393
Conversation
|
I couldn't figure out the best area label to add to this PR. Please help me learn by adding exactly one area label. |
|
/azp run runtime |
|
@jbogard thanks for preparing this pr again. :) Bringing over specification tests was in the backlog of things remaining to do. I'll look into expediting that so you could include your remaining changes into the PR. I closed and reopened the PR to just to retrigger the other builds. |
|
@jbogard I merged the PR to add the DI.Eternal tests, so you could move more of your changes now. |
|
@maryamariyan thanks, will do! |
|
@maryamariyan done! Moved over the external tests. It matches my original PR now. |
|
There was a benchmark requested too but that'll need to be in the performance repo. Right now the benchmark would just blow up. |
|
Thanks @jbogard. Looking.. |
maryamariyan
left a comment
maryamariyan
left a comment
There was a problem hiding this comment.
Aside from the question, LGTM. Thanks @jbogard for getting all the PR changes moved over to runtime repo!
| protected override IServiceProvider CreateServiceProvider(IServiceCollection serviceCollection) | ||
| public override string[] SkippedTests => new[] | ||
| { | ||
| "PublicNoArgCtorConstrainedOpenGenericServicesCanBeResolved", |
There was a problem hiding this comment.
Why are we skipping these tests? Same questions for the one in Autofac
There was a problem hiding this comment.
At one point, AutoFac didn't support constraints on new. I'm not sure that's relevant though, type constraints the useful bit for services. I'll double check though, it's been a few years since I tried this example when it failed.
The others were similar, they all worked with the desired scenarios, but not necessarily all constraints (like struct). I'll update and retry those.
|
@davidfowl @halter73 @pakrym - since there has been so much angst and discussion about this functionality in the past 3.5 years - would you mind weighing in on this PR before we merge it? |
|
Merge it. I've been concerned a about features that build more technical debt as we move to an AOT focused world. This seems like one of them. |
|
@jbogard this works with 3rd party containers right? This was the thing that caused it not to get merged before. AFAIK, that work was done right? |
|
If Autofac doesn't work then we'll need to revert... that's our most complaint container and we can't be out of sync with that. Sorry @maryamariyan and @eerhardt |
This reverts commit 211bc88.
|
sorry @jbogard this killed your third PR too. I probably should have waited for the response to my question prior to merging. That would have answered @davidfowl 's concern too. |
|
This would help solve a lot of custom plumbing in our code. Hope this will be released soon. Keep up the good work :D |
|
@danny-bos-tata sorry, we reverted this change because it doesn't work with all other DI providers |
|
New New pull request made, with packages updated, all pass the specification tests added. |
ghost
locked as resolved and limited conversation to collaborators
7 participants
Previous pull request: dotnet/extensions#536
Original discussion: aspnet/DependencyInjection#471
Original pull request: aspnet/DependencyInjection#635
3rd time's the charm?
I did not port the benchmark or the specification tests, as it looks like those weren't moved over that I could see.