-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
feat: implement low-level dependency injection container #9666
Conversation
Visit https://dashboard.github.orijtech.com?pr=9666&repo=cosmos%2Fcosmos-sdk to see benchmark details. |
autoGroupTypes map[reflect.Type]bool | ||
onePerScopeTypes map[reflect.Type]bool | ||
|
||
// logging |
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.
I would extract the logging and the graphing from the options struct. I see the config used only to configure the container. As it is done when constructing with autoGroupTypes and onePerScope types.
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.
Where I would put them instead?
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.
The alternative would be adding a parameter or overload to Run
just with debugging options separate from the container options. But that feels relatively involved. If you think it's useful to separate how about we align on an API and do that in a follow-up PR?
Co-authored-by: Marie Gauthier <[email protected]>
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
Codecov Report
@@ Coverage Diff @@
## master #9666 +/- ##
==========================================
- Coverage 63.73% 63.65% -0.09%
==========================================
Files 572 573 +1
Lines 53966 53761 -205
==========================================
- Hits 34397 34222 -175
+ Misses 17612 17590 -22
+ Partials 1957 1949 -8
|
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. No strong opinion on https://github.com/cosmos/cosmos-sdk/pull/9666/files#r710071800. But I tend to agree to split logging/graphing functionality from the logic functionality, since overall what happens is not easy to follow I'd try to keep the functionality scope of components as small as possible.
Description
closes #9775
needs #9658
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
to the type prefix if API or client breaking changeCHANGELOG.md
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!
in the type prefix if API or client breaking change