validator_os returns Result<(), OsString>

[ssokolow]

Affected Version of clap

2.20.3 and anything API-compatible with it.

Expected Behavior Summary

Callbacks for validator_os should return Result<T, String> for some T because:

• Error messages are intended for display
• It's consistent with callbacks for validator in a context where the usage doesn't differ
• It's easy to incorporate OsStr/OsString into format! output via the Debug trait.
• There's no facility for formatting natively with OsStr/OsString
• There's apparently no good reason to use Result<T, OsString>

Actual Behavior Summary

• I have to stick .into() onto the end of every Err case that I return from a path-handling validator.
• I get questions and then "eww" reactions when people not familiar with clap encounter Result<(), OsString>.

Sample Code or Link to Sample Code

pub fn filename_valid(value: &OsStr) -> Result<(), OsString> {
    // TODO: Switch to using to_bytes() once it's stabilized
    if value.to_string_lossy().chars().any(|c| is_bad_for_fname(&c)) {
        Err(format!("Name contains invalid characters: {:?}", value).into())
    } else {
        Ok(())
    }
}

I think this should be reconsidered for clap 3.x

[kbknapp]

Hmm, I see your point about errors being designed for display anyways. I'm inclined to agree it should be changed to Result<(), String> in 3.x. I'd be curious if anyone has a use case where returning a String wouldn't be acceptable?

I'll tentatively mark this for change in 3.x, but am open to discussion.

For now, .into() will have to do 😞

[kbknapp]

This has been implemented on v3-master