-
Notifications
You must be signed in to change notification settings - Fork 516
Implement Serialize & Deserialize for Sampler #622
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
Implement Serialize & Deserialize for Sampler #622
Conversation
|
Codecov Report
@@ Coverage Diff @@
## main #622 +/- ##
==========================================
- Coverage 62.08% 61.97% -0.12%
==========================================
Files 95 95
Lines 7681 7681
==========================================
- Hits 4769 4760 -9
- Misses 2912 2921 +9
Continue to review full report at Codecov.
|
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.
👍 Overall looks good. Nice job! The only thing we might want to examine is the current serialization/de-serialization process eliminated the ParentBased
variant. I don't think it would cause any issue but I think the serialization process shouldn't change the variant.
omg what am I doing. Why do I even bother implementing them manually 🤯 Oh I know... I had in my own crate because I don't own the type. Then I copied here thinking I had too. @TommyCpp this should be much easier to review. I'm not sure the test is useful at all. I suggest we delete it entirely. |
Looks good. Thanks |
Hey @TommyCpp I tried to lock the version on my side but it's actually very complicated. Because it's linked to tracing I have to fork tracing too and replace all the tracin dependencies in my project. Would it be possible for you to make a release? |
@TommyCpp never mind, I just tried with the patch syntax instead of pinning the version in the dependency and it worked like a charm. The release can wait :) |
Hello 👋
I noticed that a few types like Resource and KeyValue implement
Serialize
andDeserialize
which is very handy for me because I can integrate the differentparameter into a configuration struct.
Unfortunately it is not yet implemented for
Sampler
so I had to implement iton my crate but I think this could be beneficial for others.
Let me know what you think