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

error-stack add serde based serialization #1290

Merged
merged 26 commits into from
Nov 22, 2022
Merged

error-stack add serde based serialization #1290

merged 26 commits into from
Nov 22, 2022

Conversation

indietyp
Copy link
Member

@indietyp indietyp commented Oct 30, 2022

🌟 What is the purpose of this PR?

Create a serde serialization implementation for Report.

🔗 Related links

🔍 What does this change?

  • non-derive based serde::Serialize implementation, which is gated behind a new feature flag

📜 Does this require a change to the docs?

Yes, while teased in the CHANGELOG.md, the lib.rs documentation and module level documentation need to be adjusted.

🐾 Next steps

  • Tests
  • (Follow-Up PR): Hooks + Documentation

🛡 What tests cover this?

New snapshot tests that are located in tests/test_ser.rs

📹 Demo

[
  {
    "value": "context A",
    "attachments": [
      "printable C: 4",
    ],
    "sources": [
      {
        "value": "root error",
        "attachments": [
          "printable C: 1",
          "printable C: 3",
        ],
        "sources": [],
      },
      {
        "value": "root error",
        "attachments": [
          "printable C: 2",
          "printable C: 3",
        ],
        "sources": [],
      },
    ],
  },
]

@codecov
Copy link

codecov bot commented Oct 30, 2022

Codecov Report

Merging #1290 (4606aa9) into main (f3025f9) will increase coverage by 0.24%.
The diff coverage is 96.38%.

@@            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              
Flag Coverage Δ
backend-integration-tests 3.20% <ø> (ø)
error-stack 90.76% <96.38%> (+0.31%) ⬆️
unit-tests 1.17% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/libs/error-stack/src/lib.rs 38.46% <ø> (ø)
packages/libs/error-stack/src/serde.rs 96.38% <96.38%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@indietyp
Copy link
Member Author

(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/[email protected]
#	packages/libs/error-stack/tests/snapshots/[email protected]
#	packages/libs/error-stack/tests/snapshots/[email protected]
#	packages/libs/error-stack/tests/snapshots/[email protected]
#	packages/libs/error-stack/tests/snapshots/[email protected]
#	packages/libs/error-stack/tests/snapshots/[email protected]
#	packages/libs/error-stack/tests/snapshots/[email protected]
#	packages/libs/error-stack/tests/snapshots/test_debug__sources_nested_alternate.snap
#	packages/libs/error-stack/tests/snapshots/[email protected]
#	packages/libs/error-stack/tests/snapshots/[email protected]
#	packages/libs/error-stack/tests/snapshots/[email protected]
#	packages/libs/error-stack/tests/snapshots/test_debug__sources_nested_alternate@spantrace-backtrace-pretty-print.snap
#	packages/libs/error-stack/tests/snapshots/[email protected]
#	packages/libs/error-stack/tests/snapshots/[email protected]
#	packages/libs/error-stack/tests/snapshots/[email protected]
Copy link
Member

@TimDiekmann TimDiekmann left a 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.

Copy link
Member

@TimDiekmann TimDiekmann left a 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 🎉

TimDiekmann
TimDiekmann previously approved these changes Nov 22, 2022
@TimDiekmann
Copy link
Member

Thanks @indietyp for implementing this feature, I'm super excited about that!

@indietyp indietyp enabled auto-merge (squash) November 22, 2022 13:12
@indietyp indietyp merged commit 3764ebe into main Nov 22, 2022
@indietyp indietyp deleted the bm/error-stack/serde branch November 22, 2022 13:32
@TimDiekmann TimDiekmann added the meta/changelog Needs to be added to a changelog label Nov 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/libs > error-stack Affects the `error-stack` crate (library) meta/changelog Needs to be added to a changelog
Development

Successfully merging this pull request may close these issues.

2 participants