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

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

Merged
merged 1 commit into from
May 24, 2024

Conversation

DerickEddington
Copy link
Contributor

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.

Copy link

netlify bot commented May 18, 2024

Deploy Preview for shepmaster-snafu ready!

Name Link
🔨 Latest commit 53c4f63
🔍 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.

@shepmaster shepmaster added bug Something isn't working found in the field A user of SNAFU found this when trying to use it labels May 24, 2024
Copy link
Owner

@shepmaster shepmaster left a comment

Choose a reason for hiding this comment

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

Thank you for such a thoughtful commit message!

@shepmaster shepmaster merged commit cec2d01 into shepmaster:main May 24, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working found in the field A user of SNAFU found this when trying to use it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Report print doesn't prefix with "Error: "
2 participants