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

Add Report for user-friendly error output #355

Merged
merged 1 commit into from Oct 8, 2022
Merged

Add Report for user-friendly error output #355

merged 1 commit into from Oct 8, 2022

Conversation

shepmaster
Copy link
Owner

No description provided.

@shepmaster shepmaster added enhancement New feature or request feedback requested User feedback desired on a design decision labels Sep 27, 2022
@netlify
Copy link

netlify bot commented Sep 27, 2022

Deploy Preview for shepmaster-snafu ready!

Name Link
🔨 Latest commit acac2d5
🔍 Latest deploy log https://app.netlify.com/sites/shepmaster-snafu/deploys/633a0d97b486f00008c55f14
😎 Deploy Preview https://deploy-preview-355--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 settings.

Copy link
Collaborator

@Enet4 Enet4 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 working on this! It's been a gap in the library which so far I had to fill with custom functions.

Some early feedback inline.

src/report.rs Outdated
Comment on lines 211 to 221
for source in chain {
writeln!(f, "Caused by: {}", source)?;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is opinionated, but the repetition of "Caused by:" is a bit annoying. I am more fond of showing an enumerated list of causes.

Suggested change
for source in chain {
writeln!(f, "Caused by: {}", source)?;
}
writeln!(f, "Caused by:")?;
for (i, source) in chain.into_iter().enumerate() {
writeln!(f, " {}: {}", i, source)?;
}

It's the different between this

[ERROR] Could not initialize SCU

Caused by: could not connect to server
Caused by: No route to host (os error 113)

And this

[ERROR] Could not initialize SCU

Caused by:
   0: could not connect to server
   1: No route to host (os error 113)

Copy link
Owner Author

Choose a reason for hiding this comment

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

I'm down with that. I formatted the numbers as right-aligned with a width of 3, so it should look OK so long as you don't have 1000 nested errors...

Copy link

@8573 8573 Sep 29, 2022

Choose a reason for hiding this comment

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

Caused by:
  0: could not connect to server
  1: No route to host (os error 113)

I fear this leaves unclear the order in which the errors occurred. Reading this output (for arbitrary errors), I would think that error 0 led to error 1 (leading to error 2, and so on), but the "X, caused by Y, caused by Z" form makes clear that it's actually the reverse (error 1 led to error 0).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I fear this leaves unclear the order in which the errors occurred. Reading this output (for arbitrary errors), I would think that error 0 led to error 1 (leading to error 2, and so on), but the "X, caused by Y, caused by Z" form makes clear that it's actually the reverse (error 1 led to error 0).

Practically speaking, the order is the same. The most efficient and intuitive way of traversing the chain of sources is outer to inner, in which the root cause always appears last. All error reporting tools in Rust that I know of show the same order when printing an error report. These tools won't go out of their way to show this information in the opposite order.

Not to mention that the meaning of each error message is often enough to disambiguate the intended order of interpretation (e.g. "No route to host (os error #)" makes sense as a cause for not being able to connect to the server, whereas the opposite does not).

I disagree that this format is confusing.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Recall that this cause trace won't be shown in isolation; it's right after the outermost error:

The highest level

Caused by:
  0: The middle level
  1: The lowest level

That's from this code:

#[derive(Debug, Snafu)]
#[snafu(display("The lowest level"))]
struct Inner;

#[derive(Debug, Snafu)]
#[snafu(display("The middle level"))]
struct Middle {
    source: Inner,
}

#[derive(Debug, Snafu)]
#[snafu(display("The highest level"))]
struct Outer {
    source: Middle,
}

If the trace did go in the other direction, it'd result in an abrupt switch in direction:

The highest level

Caused by:
  0: The lowest level
  1: The middle level

These are all arguments for why the proposed formatting is "obviously correct", but I'm open to attempting to clarify the wording if possible. For example, since we only say it once, would expanding the "caused by" text be helpful at all? As a starting example:

The highest level

This was caused by these errors, in order:
  0: The middle level
  1: The lowest level

Copy link
Owner Author

Choose a reason for hiding this comment

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

The most efficient [...] way of traversing the chain of sources is outer to inner

Note that the opposite is true for the backtrace, where the first item is "where the problem occurred" and the last item is "the main method". This is unfortunate when both the backtrace and the source chain are shown at the same time.

Copy link

Choose a reason for hiding this comment

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

This was caused by these errors, in order:
 0: The middle level
 1: The lowest level

I still think "these errors, in order" suggests that the errors are listed in the order in which they were raised. What about "This was caused by these earlier errors (with the earliest error listed last):"?

Copy link
Contributor

@CryZe CryZe Sep 29, 2022

Choose a reason for hiding this comment

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

Considering this is meant to be user-friendly, can we also format it as

1. Some error
2. Some other error
3. ....

and not 0: Some error?

Copy link
Owner Author

Choose a reason for hiding this comment

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

What about "This was caused by these earlier errors (with the earliest error listed last):"?

Seems reasonable; I picked a slight modification:"Caused by these errors (earliest error listed last):".

I wonder if

  1. we should negate the parenthetical... "latest error listed first".
  2. we should do the right English thing and print "this error" when appropriate.

can we also format it as

Yeah, I thought about that briefly when writing it, so I agree.

src/report.md Outdated Show resolved Hide resolved
@Stargateur
Copy link
Contributor

Should there be a note saying the specific format is subject to change and shouldn't be expected to not change ?

Copy link
Collaborator

@Enet4 Enet4 left a comment

Choose a reason for hiding this comment

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

"Caused by these errors (recent errors listed first)" is a bit of a mouthful, but I consent.

Save for the failing test (suggested a fix inline), I have no objections to the PR as is. I will wait for this new API to be released with great anticipation!

src/report.rs Show resolved Hide resolved
@shepmaster
Copy link
Owner Author

Should there be a note saying the specific format is subject to change and shouldn't be expected to not change ?

Good idea, added.

@shepmaster shepmaster force-pushed the report branch 2 times, most recently from 3cecd9a to 0fde627 Compare October 2, 2022 22:04
@shepmaster shepmaster mentioned this pull request Oct 4, 2022
@shepmaster shepmaster linked an issue Oct 4, 2022 that may be closed by this pull request
@shepmaster shepmaster merged commit e2b29c3 into main Oct 8, 2022
@shepmaster shepmaster deleted the report branch October 8, 2022 02:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request feedback requested User feedback desired on a design decision
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Termination
5 participants