-
Notifications
You must be signed in to change notification settings - Fork 206
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
Intercept provided values #409
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #409 +/- ##
==========================================
- Coverage 98.40% 98.29% -0.12%
==========================================
Files 22 22
Lines 1507 1524 +17
==========================================
+ Hits 1483 1498 +15
- Misses 15 17 +2
Partials 9 9 ☔ View full report in Codecov by Sentry. |
Hey @asmeikal - thanks for working with us to get this to this state! One thing I think warrants discussion: Giving access to the actual 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 |
callback.go
Outdated
@@ -32,6 +34,10 @@ type CallbackInfo struct { | |||
// function, if any. When used in conjunction with [RecoverFromPanics], | |||
// this will be set to a [PanicError] when the function panics. | |||
Error error | |||
|
|||
// Values contains all values constructed by the [Callback]'s | |||
// associated function. |
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.
Given the discussion, let's add some explicit documentation here that mentions the undefined results of modifying these values.
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 updated the comment!
Co-authored-by: Jacob Oaks <[email protected]>
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: #410 |
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!