validator_os takes &OsStr, but validator takes String

[oli-obk]

If find it somewhat inconsistent that one function borrows the value, while the other takes it by value. It is very unlikely that anyone just forwards the argument passed to the Err return value, so there's no benefit of offering users an owned String.

Full disclosure: It clashes with a clippy lint, and I don't know how to act on it: rust-lang/rust-clippy#2434

[kbknapp]

This is kind of related to #848 and I plan on re-working the signature of these methods in v3 (see #1037).

The reason validator takes String by value is there is (was?) no way to convert from OsStr (which originates from env::args_os) to &str because of the utf-8 validation. But validator_os has no such stipulation and can just use the OsStr directly.

[kbknapp]

Bottom line, this is something I've been wanting to change and put more thought into but couldn't without a breaking change. Hindsight is always 20/20 and I wish I'd done it differently prior 😄

[CreepySkeleton]

Having looked at the the code, this is the only place validators play:

clap/src/parse/validator.rs

Lines 138 to 159 in 5b9dbee

	if let Some(ref vtor) = arg.validator {
	debug!("Validator::validate_arg_values: checking validator...");
	if let Err(e) = vtor(val.to_string_lossy().into_owned()) {
	debug!("error");
	return Err(Error::value_validation(Some(arg), &e, self.p.app.color())?);
	} else {
	debug!("good");
	}
	}
	if let Some(ref vtor) = arg.validator_os {
	debug!("Validator::validate_arg_values: checking validator_os...");
	if let Err(e) = vtor(val) {
	debug!("error");
	return Err(Error::value_validation(
	Some(arg),
	&(*e).to_string(),
	self.p.app.color(),
	)?);
	} else {
	debug!("good");
	}
	}

Essentially, we could just replace

vtor(val.to_string_lossy().into_owned())

with

vtor(val.to_string_lossy().as_ref())) // what was the correct function?

And change the signature for Validator.

This kind of change won't give us any perf improvement, but it will make the API uniform (both os and str versions will take a reference) and will open road to future improvements. @pksunkara What do you think?

Side note: as of now, if both validator and validator_os are present, str version takes priority (is executed first). I never saw anybody using both, but maybe me should disallow this? Or perhaps run os version only if str has failed?

[kbknapp]

Side note: as of now, if both validator and validator_os are present, str version takes priority (is executed first). I never saw anybody using both, but maybe me should disallow this? Or perhaps run os version only if str has failed?

I don't think it would make sense to have both, as all validator_os can handle anything validator can, but the reverse is not true. So I'm not sure there'd be a reason for a user to have both?

[CreepySkeleton]

None I can think of. I'll make it panic in case both have been passed.