feat: introduce resolve-request abstraction #1188
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #1188 +/- ##
===========================================
+ Coverage 80.23% 80.25% +0.02%
===========================================
Files 187 187
Lines 4371 4371
Branches 947 947
===========================================
+ Hits 3507 3508 +1
Misses 414 414
+ Partials 450 449 -1
Continue to review full report at Codecov.
|
|
@autofac-bot benchmark DeepGraphResolveBenchmark |
alsami
changed the title
DeepGraphResolveBenchmarkTarget: develop
Source: feat/introduce_abstraction_for_resolve_request
|
|
@autofac-bot benchmark DeepGraphResolveBenchmark |
DeepGraphResolveBenchmarkTarget: develop
Source: feat/introduce_abstraction_for_resolve_request
|
…DefaultResolveRequest
|
@autofac-bot benchmark DeepGraphResolveBenchmark |
DeepGraphResolveBenchmarkTarget: develop
Source: feat/introduce_abstraction_for_resolve_request
|
|
@autofac-bot benchmark DeepGraphResolveBenchmark |
DeepGraphResolveBenchmarkTarget: develop
Source: feat/introduce_abstraction_for_resolve_request
|
|
@tillig @alistairjevans basically ready for review. I ran the benchmarks initially using the bot but the differences were too big comparing develop and PR branch. I did that for the first commit (twice), which introduced an interface. For the second commit as well, which kind of uses the same principle that we have introduced for develop:
PR branch:
Any thought's on that? Not sure if I should try the interfaces again now. Also, does a difference of 0.15 us make a big difference? 🤔 |
|
What is the motivation behind making ResolveRequest an interface / abstract class? I ask because it is a very small type, it can be relatively easily mocked (especially now), and it is very well established as a type you can instantiate and do stuff with. It's going to introduce a lot of extra breaking change. Might I propose that we have a go updating the unit tests with the changes made so far before we do more refactoring? It would be good to only change ResolveRequest if we absolutely have to. |
Basically the requirement to make testing easier and only really have to mock what you need, since it has one constructor with all the parameters required.
I agree, this class has been public for a very long time probably. Most of the parameters can be rather easily mocked as well, but still require mocking.
We actually already did that, if you are talking about |
|
I'm thinking about things like ActivatorPipelineExtensions, which has been changed for the new naming, but not properly refactored to take advantage of the new abstract classes. That class only exists because of how complex it used to be to mock a pipeline invocation. I feel like we should try reworking things to use the new abstract classes before we do more refactoring. We may have already made enough changes to satisfy #1164. |
Okay cool, will look into it.
Fine for me, I've just understood that it was kind of a nice to have to make it even easier in the specific area. Closing this for now |
|
Thanks; it might be we still need this, but it would be good to know for sure. |
related to #1164