Skip to content
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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

asmeikal
Copy link

@asmeikal asmeikal commented Mar 4, 2024

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!

Copy link

codecov bot commented Mar 4, 2024

Codecov Report

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).

❗ 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 ⚠️
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.
📢 Have feedback on the report? Share it here.

@JacobOaks
Copy link
Contributor

JacobOaks commented Mar 7, 2024

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.Values 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.

@tchung1118
Copy link
Contributor

I agree with @JacobOaks that this is probably fine, but we should clearly document that those values in CallbackInfo shouldn't be modified.

result.go Outdated Show resolved Hide resolved
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.
Copy link
Contributor

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the comment!

@JacobOaks
Copy link
Contributor

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

@CLAassistant
Copy link

CLA assistant check
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.

✅ asmeikal
❌ Michelle Laurenti


Michelle Laurenti seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants