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

Implement Termination for error_stack::Report #671

Merged
merged 7 commits into from
Jun 21, 2022

Conversation

gluax
Copy link
Contributor

@gluax gluax commented Jun 20, 2022

🌟 What is the purpose of this PR?

This adds custom error code support for error-stack

🔗 Related links

Closed #670.

Though the implementation has changed since I wrote the issue and learned more about the code base.

🔍 What does this change?

  • Adds a method attach_exit_code(mut self, exit_code: u8) -> Self on the Report<C> struct.
  • Adds a new field exit_code: Option<u8> on the ReportImpl struct.
  • Implements the Termination trait for the Report<C> struct.

📜 Does this require a change to the docs?

The docs have been added.

🐾 Next steps

Please let me know if there are any changes that should be made.

🛡 What tests cover this?

There is the test doc test that passes. Let me know if I should add a separate test.

❓ How to test this?

  1. Pull the forked repo.
  2. Check out the feature/custom-exit-code branch.
  3. Run cargo test --doc

📹 Demo

image

@CLAassistant
Copy link

CLAassistant commented Jun 20, 2022

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added the area/libs > error-stack Affects the `error-stack` crate (library) label Jun 20, 2022
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.

Hi @gluax and thank you very much for the contribution!

I really like the idea of implementing Termination on Report. However, I think the API can be simplified (see comments). This sadly requires a nightly compiler to really use ExitCode, but as soon as the Provider API is stabilized, this would also be available on stable.

packages/libs/error-stack/src/report.rs Outdated Show resolved Hide resolved
packages/libs/error-stack/src/report.rs Outdated Show resolved Hide resolved
@vilkinsons
Copy link
Member

@gluax This is awesome, thanks for contributing!

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.

Thanks for applying the suggestions!

Would be great if you could add a small test checking for the right exit code. We don't have a tests module in report.rs yet, you may add one at the bottom of the file. 👍🏻

packages/libs/error-stack/src/report.rs Outdated Show resolved Hide resolved
packages/libs/error-stack/src/report.rs Outdated Show resolved Hide resolved
@gluax
Copy link
Contributor Author

gluax commented Jun 21, 2022

Example and test added! As well as the other suggested changes.

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.

Only a few suggestions that you should be able to apply directly from GitHub, otherwise it seems to be ready for merging.

In order to merge, the Rust jobs and the CI-linting job has to pass, others can be ignored (they fail because of insufficient permissions, nothing you should need to worry about).

packages/libs/error-stack/Cargo.toml Outdated Show resolved Hide resolved
packages/libs/error-stack/Cargo.toml Outdated Show resolved Hide resolved
packages/libs/error-stack/src/report.rs Outdated Show resolved Hide resolved
TimDiekmann added a commit that referenced this pull request Jun 21, 2022
@TimDiekmann TimDiekmann added the meta/changelog Needs to be added to a changelog label Jun 21, 2022
gluax and others added 3 commits June 21, 2022 08:19
Co-authored-by: Tim Diekmann <21277928+TimDiekmann@users.noreply.github.com>
Co-authored-by: Tim Diekmann <21277928+TimDiekmann@users.noreply.github.com>
Co-authored-by: Tim Diekmann <21277928+TimDiekmann@users.noreply.github.com>
@TimDiekmann TimDiekmann changed the title feat(error-stack): custom error codes Implement Termination(https://doc.rust-lang.org/stable/std/process/trait.Termination.html) for Report Jun 21, 2022
@TimDiekmann TimDiekmann changed the title Implement Termination(https://doc.rust-lang.org/stable/std/process/trait.Termination.html) for Report Implement Termination for error_stack::Report Jun 21, 2022
@TimDiekmann TimDiekmann enabled auto-merge (squash) June 21, 2022 15:29
@TimDiekmann
Copy link
Member

Thank you very much for your contribution, I really appreciate it! 🎉

@TimDiekmann TimDiekmann merged commit ca30ff2 into hashintel:main Jun 21, 2022
teenoh pushed a commit that referenced this pull request Jun 27, 2022
@vilkinsons vilkinsons added category/enhancement New feature or request and removed C-enhancement labels Jul 20, 2023
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) category/enhancement New feature or request meta/changelog Needs to be added to a changelog
Development

Successfully merging this pull request may close these issues.

error-stack: custom exit codes
4 participants