clap-rs / clap

A full featured, fast Command Line Argument Parser for Rust
docs.rs/clap
Apache License 2.0
14.06k stars 1.03k forks source link

Duplicated "error:" prefix #5579

Open d-e-s-o opened 2 months ago

d-e-s-o commented 2 months ago

Please complete the following tasks

Rust Version

rustc 1.79.0-nightly (129f3b996 2024-06-10) (gentoo)

Clap Version

4.5.4

Minimal reproducible code

use std::env::args_os;
use anyhow::Result;
use clap::Parser;

/// Simple program to greet a person
#[derive(Parser, Debug)]
#[command(version, about, long_about = None)]
struct Args {
    /// Name of the person to greet
    #[arg(short, long)]
    name: String,
}

fn main() -> Result<()> {
    let _args = Args::try_parse_from(args_os())?;
    Ok(())
}

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=6221486eaf814c633157f1c9fdf27c12

Steps to reproduce the bug with the above code

cargo run

Actual Behaviour

Prints:

Error: error: the following required arguments were not provided:
  --name <NAME>

Usage: playground --name <NAME>

For more information, try '--help'.

Expected Behaviour

Prints:

Error: the following required arguments were not provided:
  --name <NAME>

Usage: playground --name <NAME>

For more information, try '--help'.

Additional Context

The "error:" prefix is duplicated. I find this behavior irritating. One prefix seems to be added by the standard library, the other by clap (I don't believe clap is alone with such behavior)

In my opinion the standard library's behavior is questionable -- it's just way too opinionated and this is a good example -- but I wanted to get your guys' take and have an issue to reference to make the case for changing it (which I suppose may be a hard sell irrespectively).

In general, though, it also feels as if just the typical wording of "failed to [...]" or similar would be sufficient for convey the issue. Meaning that there is no need for an "error:" at all, even from the clap side. It just depends too much on the context how the error is reported whether it makes sense to include such a prefix or not.

Any opinions/comments/thoughts?

Debug Output

No response

epage commented 2 months ago

Is there a reason you are using try_parse_from, rather than parse?

d-e-s-o commented 2 months ago

Is there a reason you are using try_parse_from, rather than parse?

Yep.

epage commented 1 month ago

This isn't the only peculiarity about claps' error reporting that doesn't make it work well with policies like anyhow. For example, you likely should condition your exit code based on er.use_stderr() (not a great name) or else --help is considered an error when its generally recognized as not one.

We need more information to better understand your use case to determine what, if anything, we should be done about this.

d-e-s-o commented 1 month ago

This has very little if not nothing to do with anyhow from what I can tell.

My use case is using regular error reporting paths as every other crate does and have a single program exit path. If every crate thinks it knows best when to exit then I have no control over or idea of what is going on. The same is true (and, to the best I can tell, accepted practice) for panics: you can unwrap or panic on invariants and everything else (certainly everything controllable by a user) shouldn't panic, but report a handleable error.

I assume that is the reason why the try_parse_from API exist in the first place, right?