search

shepmaster / snafu

Easily assign underlying errors into domain-specific errors while adding context
https://docs.rs/snafu/
Apache License 2.0
1.39k stars 60 forks source link

Make `Report::report` print the "Error: " prefix #453

Closed DerickEddington closed 3 months ago

DerickEddington commented 3 months ago

Fixes #452

My reasoning to address the concern mentioned in the issue:

we just need to ensure that we don't end up printing Error: Error: ... when older versions of Rust are targeted.

It seems to me that is sufficiently ensured:

  1. When #[snafu::report] transforms main, or a #[test] function, to return -> Result<(), Report<E>>, to support older versions of Rust (i.e. when the package feature rust_1_61 is not enabled), or when that same return type was chosen explicitly (i.e. without using the attribute-macro), it is instead Result as Termination that is used. That different implementation, with the Error: prefix printed by it instead, uses the Debug formatting of Report (which is the same as its Display impl), for which SNAFU already does not add any prefix. So, it's impossible for an extra prefix to be printed due to my change, for all preexisting and future uses of <Result<T, Report<E>> as Termination>::report.

  2. Only <Report as Termination>::report is changed, and so the only way extra Error: (or the like) could be printed is if a user chooses to add their own such extra print before that function is called.

    1. Given the narrow purpose of that function, it seems very unlikely that anyone is calling it other than implicitly after main, or a #[test] fn, returns.

      1. Even if some user has explicitly called <T as Termination>::report in a generic context anywhere (e.g. in the body of some test of that functionality), it seems likely they'd already be prepared for it to print the prefix, because the std library already does that for Result, and so they wouldn't have added their own printing of a prefix.
      2. It seems unlikely that anyone is explicitly calling only the specific concrete <Report as Termination>::report, but if they are then they shouldn't have relied on it to not print the prefix, because SNAFU's documentation already implies that it will print the prefix.

    2. If any user has been printing their own prefix before main, or a #[test] fn, returns, then they should already be expecting an extra prefix, because without feature rust_1_61 (e.g. for older versions of Rust) the Error: prefix was already being printed by the std lib's <Result<(), Report<E>> as Termination>::report, and so such users should not have expected it to not continue doing that with newer Rust versions when <Report as Termination>::report is used in what was intended to be an equivalent internal-detail change.

  3. SNAFU's documentation says:

    The exact content and format of a displayed Report are not stable

I added a test to ensure that <Report as Display>::fmt continues to not add an "error" prefix, to ensure that point 1 continues to be upheld.

I tried to think if any other tests should be added for any other aspects of this, but couldn't think of anything, because it's not possible to redirect the stderr output of a #[test] function to check it, nor did I notice any preexisting facility for checking the output of a test program (in contrast to the preexisting compatibility-tests/compile-fail/), as would be needed to test that the prefix is now being printed.

netlify[bot] commented 3 months ago

Deploy Preview for shepmaster-snafu ready!

Name Link
Latest commit 53c4f63f5d610bfdcdf29129591d6469751fb8e8
Latest deploy log https://app.netlify.com/sites/shepmaster-snafu/deploys/6650b9af66f42e0008d00511
Deploy Preview https://deploy-preview-453--shepmaster-snafu.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.