-
Notifications
You must be signed in to change notification settings - Fork 110
error-stack add serde based serialization
#1290
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
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1290 +/- ##
==========================================
+ Coverage 44.58% 44.83% +0.24%
==========================================
Files 315 316 +1
Lines 17248 17331 +83
Branches 813 813
==========================================
+ Hits 7690 7770 +80
- Misses 9553 9556 +3
Partials 5 5
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
(Tests fail because the location for some snapshots changed, this will be fixed as soon as I am able to do so) |
# Conflicts: # packages/libs/error-stack/tests/common.rs # packages/libs/error-stack/tests/snapshots/test_debug__full__alt.snap # packages/libs/error-stack/tests/snapshots/test_debug__full__complex.snap # packages/libs/error-stack/tests/snapshots/test_debug__full__hook.snap # packages/libs/error-stack/tests/snapshots/test_debug__full__hook_context.snap # packages/libs/error-stack/tests/snapshots/test_debug__full__hook_decr.snap # packages/libs/error-stack/tests/snapshots/test_debug__full__hook_for_context.snap # packages/libs/error-stack/tests/snapshots/test_debug__full__hook_incr.snap # packages/libs/error-stack/tests/snapshots/test_debug__full__hook_multiple.snap # packages/libs/error-stack/tests/snapshots/test_debug__full__hook_provider.snap # packages/libs/error-stack/tests/snapshots/test_debug__full__linear.snap # packages/libs/error-stack/tests/snapshots/test_debug__full__linear_ext.snap # packages/libs/error-stack/tests/snapshots/test_debug__full__multiline.snap # packages/libs/error-stack/tests/snapshots/test_debug__full__norm.snap # packages/libs/error-stack/tests/snapshots/test_debug__full__sources.snap # packages/libs/error-stack/tests/snapshots/test_debug__full__sources_transparent.snap # packages/libs/error-stack/tests/snapshots/test_debug__sources_nested.snap # packages/libs/error-stack/tests/snapshots/test_debug__sources_nested@backtrace-pretty-print.snap # packages/libs/error-stack/tests/snapshots/test_debug__sources_nested@backtrace.snap # packages/libs/error-stack/tests/snapshots/test_debug__sources_nested@pretty-print.snap # packages/libs/error-stack/tests/snapshots/test_debug__sources_nested@spantrace-backtrace-pretty-print.snap # packages/libs/error-stack/tests/snapshots/test_debug__sources_nested@spantrace-backtrace.snap # packages/libs/error-stack/tests/snapshots/test_debug__sources_nested@spantrace-pretty-print.snap # packages/libs/error-stack/tests/snapshots/test_debug__sources_nested@spantrace.snap # packages/libs/error-stack/tests/snapshots/test_debug__sources_nested_alternate.snap # packages/libs/error-stack/tests/snapshots/test_debug__sources_nested_alternate@backtrace-pretty-print.snap # packages/libs/error-stack/tests/snapshots/test_debug__sources_nested_alternate@backtrace.snap # packages/libs/error-stack/tests/snapshots/test_debug__sources_nested_alternate@pretty-print.snap # packages/libs/error-stack/tests/snapshots/test_debug__sources_nested_alternate@spantrace-backtrace-pretty-print.snap # packages/libs/error-stack/tests/snapshots/test_debug__sources_nested_alternate@spantrace-backtrace.snap # packages/libs/error-stack/tests/snapshots/test_debug__sources_nested_alternate@spantrace-pretty-print.snap # packages/libs/error-stack/tests/snapshots/test_debug__sources_nested_alternate@spantrace.snap
TimDiekmann
left a comment
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.
@indietyp and I spoke about the exact representation offline and after a few back and forth agreed on a similar representation as we use internally. The contexts will be nested as this is the most intuitive format when a Report is serialized. This has the disadvantage, that long chains of errors will get bigger nesting, but we agreed that serialized reports are mainly used internally, and parsing those should be as straightforward as possible. To pretty print a report, the Debug implementation typically will be used.
The attachments will remain a list inside of each context.
TimDiekmann
left a comment
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.
This looks very good, thank you, I'm super excited about this! Sorry, that it took me so long to review this 🙁
Only have a few comments on the implementation details, the output LGTM 🎉
Co-authored-by: Tim Diekmann <21277928+TimDiekmann@users.noreply.github.com>
|
Thanks @indietyp for implementing this feature, I'm super excited about that! |
Co-authored-by: Tim Diekmann <21277928+TimDiekmann@users.noreply.github.com>
🌟 What is the purpose of this PR?
Create a serde serialization implementation for
Report.🔗 Related links
serdeimplementation forerror-stack#792: Previous PR, due to internal changes closed in favor of this one🔍 What does this change?
serde::Serializeimplementation, which is gated behind a new feature flag📜 Does this require a change to the docs?
Yes, while teased in the
CHANGELOG.md, thelib.rsdocumentation and module level documentation need to be adjusted.🐾 Next steps
🛡 What tests cover this?
New snapshot tests that are located in
tests/test_ser.rs📹 Demo