blyxxyz / lexopt

Minimalist pedantic command line parser
MIT License
294 stars 9 forks source link

Do not use `Error::source` in `fmt::Display` implementation #22

Open woojiq opened 7 months ago

woojiq commented 7 months ago

What I mean by this title is that if I want to manually print the entire error stack using the Error::source method, I get an error message twice. Example:

cannot parse argument "213": The length of hex string must be 6
The length of hex string must be 6

Code I use to unwind an error:

let args = args::parse_cli_args().unwrap_or_else(|err| {
    let mut err: Box<&(dyn std::error::Error)> = Box::new(&err);
    loop {
        eprintln!("{}", err);
        if let Some(source) = err.source() {
            err = Box::new(source);
        } else {
            break;
        }
    }
    std::process::exit(1);
});

I understand that the idea behind "lexopt" is to be as simple as possible with no fuss. Therefore, I suggest not to change the current behavior much. We can move the current behavior to the "{:#}" formatter and use "{}" to print a single error message (to be able to print the full error manually). That is how anyhow does this^1. But it will be breaking changes (or not?). What do you think? If you want to see it too, I can create a PR.

https://github.com/rust-lang/project-error-handling/issues/23

For now, the recommendation is to always do one or the other. If you're going to print your sources, you should not return them via the source function and vice versa.

Cheers.

blyxxyz commented 7 months ago

I'm worried that this would make error messages worse for currently existing applications that only use Display. And it would do it in such a way that they might not realize it when upgrading. What are your thoughts on removing the source implementation instead?

I'll have to do a survey of existing code as well as some general research.

woojiq commented 7 months ago

And it would do it in such a way that they might not realize it when upgrading.

Agree. Although bump version (0.3.0 -> 0.4.0) and mention it in the changelog and I think it will be ok.

What are your thoughts on removing the source implementation instead?

It's not the worst option in the short term, but overall it doesn't look good because we could have an already composite error that lexopt takes and we lose information.