Skip to content

feat: improve error printing #517

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.

Sign up for GitHub

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

Open
m4tx wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat: improve error printing #517

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion cot-core/src/error/error_impl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,26 @@ impl Error {

impl Debug for Error {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
Debug::fmt(&self.repr, f)
// If the alternate (`{:#?}`) formatting has been specified, print out the
// `Debug` formatting normally. If not, (which is the case when using
// `Result::unwrap()` or `Result::expect()`) pretty print the error.
if f.alternate() {
Debug::fmt(&self.repr, f)
} else {
Display::fmt(&self.repr.inner, f)?;

writeln!(f)?;
writeln!(f)?;
writeln!(f, "caused by:")?;
Comment on lines +182 to +184
Copy link
Copy Markdown
Member

@seqre seqre Mar 22, 2026

Choose a reason for hiding this comment

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

I think that should be within the while loop to show the chain of causality

Copy link
Copy Markdown
Member Author

@m4tx m4tx Mar 22, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

The Rust stack traces are printed with 0:, 1:, 2:, etc. at the beginning of each frame. Maybe that would do? I don't think I'd like to inflate the error messages with excessive newlines.

let mut source = self.source();
while let Some(e) = source {
writeln!(f, " {e}")?;

Comment on lines +180 to +188
Copy link

Copilot AI Mar 22, 2026

Choose a reason for hiding this comment

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

The source-chain printing here will duplicate the top-level message for errors created via Error::with_status/Error::internal: the outer Error displays the inner Error via WithStatusCode, and then self.source() returns that same inner Error as the first source, so the first "caused by" entry repeats the root error. Consider printing from self.inner() (both the root message and the chain), or otherwise skipping the first source when it’s the status-propagation Error wrapper.

Suggested change
Display::fmt(&self.repr.inner, f)?;
writeln!(f)?;
writeln!(f)?;
writeln!(f, "caused by:")?;
let mut source = self.source();
while let Some(e) = source {
writeln!(f, " {e}")?;
// Print the root error message.
Display::fmt(&self.repr.inner, f)?;
// Prepare to print the source chain.
let root_msg = format!("{}", &self.repr.inner);
writeln!(f)?;
writeln!(f)?;
writeln!(f, "caused by:")?;
let mut source = self.source();
let mut is_first = true;
while let Some(e) = source {
let msg = format!("{}", e);
// For status-propagation wrappers, the first source may have the
// same message as the root error; skip it to avoid duplication.
if !is_first || msg != root_msg {
writeln!(f, " {msg}")?;
}
is_first = false;

Copilot uses AI. Check for mistakes.
source = e.source();
Comment on lines +182 to +189
Copy link

Copilot AI Mar 22, 2026

Choose a reason for hiding this comment

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

This always prints a blank-line separator and a "caused by:" section even when self.source() is None, which will produce output ending with an empty "caused by:" block. Consider only emitting the extra newlines and the "caused by" header when at least one source exists.

Suggested change
writeln!(f)?;
writeln!(f)?;
writeln!(f, "caused by:")?;
let mut source = self.source();
while let Some(e) = source {
writeln!(f, " {e}")?;
source = e.source();
let mut source = self.source();
if source.is_some() {
writeln!(f)?;
writeln!(f)?;
writeln!(f, "caused by:")?;
while let Some(e) = source {
writeln!(f, " {e}")?;
source = e.source();
}

Copilot uses AI. Check for mistakes.
}

Ok(())
}
Comment on lines 172 to +193
Copy link

Copilot AI Mar 22, 2026

Choose a reason for hiding this comment

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

The pretty-printing logic in Debug::fmt is new user-facing behavior but there are no tests asserting the {:#?} vs {:?} formatting, the presence/absence of the "caused by" block, and the formatting of multi-level source chains. Adding unit tests in this file’s tests module would help prevent regressions (especially around Error::internal/Error::with_status).

Copilot uses AI. Check for mistakes.
}
}

Expand Down
Loading