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

Allow conversion from Report to Error #1749

Merged
merged 7 commits into from
Jan 3, 2023
Merged

Allow conversion from Report to Error #1749

merged 7 commits into from
Jan 3, 2023

Conversation

TimDiekmann
Copy link
Member

🌟 What is the purpose of this PR?

It's currently hard to deal with Report when an Error is required. This PR adds conversions from Report to Error.

🔗 Related links

🔍 What does this change?

  • Add Report::into_error to convert impl Error
  • Add Report::as_error to return &impl Error
  • Add From implementation of Report for Box<dyn Error> (+ Send and/or Sync)

📜 Does this require a change to the docs?

The changelog was changed to reflect these changes

🛡 What tests cover this?

Three new tests were added:

  • A UI test to make sure, that into_report cannot be called on Result<T, Report<C>>
  • Two tests to check the new methods/trait implementation by converting a Report to Error and ensure, that the provided values are still accessible

@TimDiekmann TimDiekmann requested a review from a team January 3, 2023 12:47
@github-actions github-actions bot added the area/libs > error-stack Affects the `error-stack` crate (library) label Jan 3, 2023
@vercel
Copy link

vercel bot commented Jan 3, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
hash 🔄 Building (Inspect) Jan 3, 2023 at 0:52AM (UTC)
hashdotdev 🔄 Building (Inspect) Jan 3, 2023 at 0:52AM (UTC)

@codecov
Copy link

codecov bot commented Jan 3, 2023

Codecov Report

Merging #1749 (07968da) into main (1e14659) will increase coverage by 0.03%.
The diff coverage is 70.00%.

@@            Coverage Diff             @@
##             main    #1749      +/-   ##
==========================================
+ Coverage   55.42%   55.46%   +0.03%     
==========================================
  Files         223      224       +1     
  Lines       15096    15136      +40     
  Branches      378      378              
==========================================
+ Hits         8367     8395      +28     
- Misses       6724     6736      +12     
  Partials        5        5              
Flag Coverage Δ
backend-integration-tests 2.87% <ø> (ø)
error-stack 90.32% <70.00%> (-0.55%) ⬇️
unit-tests 1.66% <ø> (ø)

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/report.rs 85.22% <62.50%> (-3.59%) ⬇️
packages/libs/error-stack/src/error.rs 81.25% <81.25%> (ø)

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

impl<C> Error for ReportError<C> {
#[cfg(nightly)]
fn provide<'a>(&'a self, demand: &mut Demand<'a>) {
self.0.frames().for_each(|frame| frame.provide(demand));
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, any reason for choosing for_each over for frame in self.frames()?

Copy link
Member Author

Choose a reason for hiding this comment

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

No specific reason. I used it here as it's only one line vs three lines.

@@ -464,7 +464,6 @@
)]

extern crate alloc;
extern crate core;
Copy link
Member

Choose a reason for hiding this comment

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

I love how CLion always automatically adds this line...

Copy link
Member

@indietyp indietyp left a comment

Choose a reason for hiding this comment

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

Looks good to me, just some minor non-blocking comments. I already noted this over private DMs, but with this change

#[test]
fn as_error_nest() {
    let report = create_report();
    let base = report.into_error();
    let report = Report::new(base);

    println!("{report:?}");
}

is possible. I think that is ok, as it does not break formatting and has quite the high barrier for creating it.

@TimDiekmann
Copy link
Member Author

Thanks, @indietyp!
Yes, that's possible but this was also possible before by creating a newtype for Report and implementing the required traits on that. The barrier, however, to do this is high enough and one cannot accidentally call into_report().

@TimDiekmann TimDiekmann merged commit bc9d764 into main Jan 3, 2023
@TimDiekmann TimDiekmann deleted the td/es/into-error branch January 3, 2023 14:10
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)
Development

Successfully merging this pull request may close these issues.

Make Report compatible with std::error::Error based types
2 participants