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

GEN-37: Combine IntoReport and ResultExt #2883

Merged
merged 7 commits into from
Aug 10, 2023

Conversation

TimDiekmann
Copy link
Member

@TimDiekmann TimDiekmann commented Aug 9, 2023

🌟 What is the purpose of this PR?

After a lot of back and forth and various discussions (internally and on GitHub and other public platforms), we decided to merge the functionality of IntoReport into ResultExt.

🔗 Related links

🔍 What does this change?

  • Extend ResultExt by a type Context: Context associated type (technically a breaking change likely has no impact) - requires a minor version bump
  • Implement ResultExt for core::result::Result<T, C> where C: Context

Pre-Merge Checklist 🚀

🚢 Has this modified a publishable library?

This PR:

  • modifies a Cargo-publishable library, but it is not yet ready to publish

📜 Does this require a change to the docs?

The changes in this PR:

  • require changes to docs which are made as part of this PR

🕸️ Does this require a change to the Turbo Graph?

The changes in this PR:

  • do not affect the execution graph

⚠️ Known issues

  • We still have IntoReportCompat but due to the Orphan rule it's not possible to implement ResultExt on eyre or anyhow errors.

@TimDiekmann TimDiekmann requested a review from a team as a code owner August 9, 2023 20:26
@linear
Copy link

linear bot commented Aug 9, 2023

GEN-37

@github-actions github-actions bot added area/libs > error-stack Affects the `error-stack` crate (library) area/libs Relates to first-party libraries/crates/packages (area) area/tests New or updated tests labels Aug 9, 2023
@TimDiekmann TimDiekmann changed the title Combine IntoReport and ResultExt Gen-37: Combine IntoReport and ResultExt Aug 9, 2023
@codecov
Copy link

codecov bot commented Aug 9, 2023

Codecov Report

Attention: Patch coverage is 13.72549% with 44 lines in your changes missing coverage. Please review.

Project coverage is 51.55%. Comparing base (1752dea) to head (848e8d7).
Report is 2606 commits behind head on main.

Files with missing lines Patch % Lines
libs/error-stack/src/result.rs 13.72% 44 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2883      +/-   ##
==========================================
- Coverage   51.63%   51.55%   -0.09%     
==========================================
  Files         337      337              
  Lines       28971    29022      +51     
  Branches      433      433              
==========================================
+ Hits        14960    14963       +3     
- Misses      14009    14057      +48     
  Partials        2        2              
Flag Coverage Δ
error-stack 80.55% <13.72%> (-2.00%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@github-actions github-actions bot added the area/infra Relates to version control, CI, CD or IaC (area) label Aug 9, 2023
@vilkinsons vilkinsons changed the title Gen-37: Combine IntoReport and ResultExt GEN-37: Combine IntoReport and ResultExt Aug 9, 2023
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.

Minor things, otherwise looking good. Thank you!

libs/error-stack/src/report.rs Outdated Show resolved Hide resolved
libs/error-stack/src/result.rs Show resolved Hide resolved
libs/error-stack/src/compat.rs Outdated Show resolved Hide resolved
libs/error-stack/src/lib.rs Outdated Show resolved Hide resolved
Co-authored-by: Bilal Mahmoud <bmahmoud@mpi-cbg.de>
@indietyp
Copy link
Member

indietyp commented Aug 9, 2023

Also: We still need a changelog entry!

@TimDiekmann TimDiekmann linked an issue Aug 9, 2023 that may be closed by this pull request
indietyp
indietyp previously approved these changes Aug 9, 2023
@TimDiekmann TimDiekmann added this pull request to the merge queue Aug 9, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 9, 2023
@TimDiekmann TimDiekmann added this pull request to the merge queue Aug 10, 2023
Merged via the queue into main with commit 1b9ea2d Aug 10, 2023
27 checks passed
@TimDiekmann TimDiekmann deleted the td/gen-37-allow-change_context-to-call-into_report branch August 10, 2023 00:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/infra Relates to version control, CI, CD or IaC (area) area/libs > error-stack Affects the `error-stack` crate (library) area/libs Relates to first-party libraries/crates/packages (area) area/tests New or updated tests
Development

Successfully merging this pull request may close these issues.

Automatic conversion from errors to error stack? H-357: Allow change_context to call into_report
2 participants