-
Notifications
You must be signed in to change notification settings - Fork 716
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor resolve request #1323
Refactor resolve request #1323
Conversation
refactor: rename Binding T generic to TActivated
Nice work, LGTM
sâm., 1 mai 2021, 12:19 notaphplover ***@***.***> a scris:
… @notaphplover <https://github.com/notaphplover> requested review from
@inversify/maintainers on: #1323
<#1323> Refactor resolve
request.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#1323 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABU32VIT66DITBILSGQOFULTLPBTJANCNFSM436EJELQ>
.
|
There's a test failing on node 14 (es6), I'll have a look but I'll be missing for some hours. @tonyhallett could you have a look? EDIT: if it actually sounds rude, I'm just asking a favor 😃 (english is not my first language, sry) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! I think it's possible to reduce the complexity of those functions, I can go for it this night
@notaphplover @PodaruDragos Ok to squash and merge ? |
done making changes |
I'm back at home, on my way to review it :) |
Ok I have some changes to request, I already have them unstaged on my local repo so if you agree I can easily push them |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are the changes :)
Please do ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only other thing that could be done is to further replace T with TActivated. I personally don't mind if we do this at a later stage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I think we can squash and merge once the changelog has the BCs!
Ok can I leave that with you ? |
I said to add BC to the changelog because I read #1319, but im not sure now. I think the cache refactor task is the right one |
So, I think we can squash and merge now, Ill do it this afternoon, once Im at home :) |
Description
_resolveRequest
function has been refactored to extract a_resolveBinding
function.T
type toresolveInstance
call at _resolveRequest.Binding.factory
to beFactoryCreator<TActivated> | null
.Related Issue
#1321, #1322
Motivation and Context
This change reduces the code complexity of the _resolveRequest funcion and also adds more concrete types
How Has This Been Tested?
Types of changes
Checklist: