Open asmeikal opened 7 months ago
Attention: Patch coverage is 90.47619%
with 2 lines
in your changes are missing coverage. Please review.
Project coverage is 98.29%. Comparing base (
897df36
) to head (cd7d216
).:exclamation: Current head cd7d216 differs from pull request most recent head 16c63d6. Consider uploading reports for the commit 16c63d6 to get more accurate results
Files | Patch % | Lines |
---|---|---|
result.go | 81.81% | 2 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Hey @asmeikal - thanks for working with us to get this to this state! One thing I think warrants discussion:
Giving access to the actual reflect.Value
s returned by constructor/decorators in callback opens up opportunity for some funky stuff. For example, I think callback functions will be able to use reflect to modify the actual values stored inside containers (for addressable values at least), which could definitely cause unexpected/unsound behavior. One alternative I can think of would be to make sure that the values passed to callbacks are deep copies of those stored in the container, but I'm not sure that would be worth it.
So, my initial thoughts are that this is probably fine, as long as we aggressively plaster clear documentation that modifying these values results in undefined behavior, but I'd like to hear your thoughts & thoughts of other maintainers @tchung1118 @sywhang.
I agree with @JacobOaks that this is probably fine, but we should clearly document that those values in CallbackInfo
shouldn't be modified.
Hey there, the maintainers of Dig discussed this at some length and decided to open up a discussion to describe our concerns and solicit more thoughts: https://github.com/uber-go/dig/discussions/410
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.
:white_check_mark: asmeikal
:x: Michelle Laurenti
This is our current implementation for #408. It adds a container Option to intercept values as they are constructed, to apply additional startup logic.
The naming could certainly be improved!