Skip to content

Commit

Permalink
Allow empty value by default, introduce NoEquals Error
Browse files Browse the repository at this point in the history
  • Loading branch information
ldm0 committed Mar 10, 2021
1 parent 30d784f commit a1d4bd8
Show file tree
Hide file tree
Showing 9 changed files with 179 additions and 32 deletions.
2 changes: 1 addition & 1 deletion src/build/arg/debug_asserts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ fn assert_app_flags(arg: &Arg) {
}
}

checker!(AllowEmptyValues requires TakesValue);
checker!(NoEmptyValues requires TakesValue);
checker!(RequireDelimiter requires TakesValue | UseValueDelimiter);
checker!(HidePossibleValues requires TakesValue);
checker!(AllowHyphenValues requires TakesValue);
Expand Down
42 changes: 27 additions & 15 deletions src/build/arg/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4160,10 +4160,11 @@ impl<'help> Arg<'help> {
self.multiple_occurrences(multi).multiple_values(multi)
}

/// Allows an argument to accept explicitly empty values. An empty value must be specified at
/// the command line with an explicit `""`, `''`, or `--option=`
/// Don't allow an argument to accept explicitly empty values. An empty value
/// must be specified at the command line with an explicit `""`, `''`, or
/// `--option=`
///
/// **NOTE:** By default empty values are *not* allowed
/// **NOTE:** By default empty values are allowed.
///
/// **NOTE:** Setting this requires [`ArgSettings::TakesValue`].
///
Expand All @@ -4173,45 +4174,56 @@ impl<'help> Arg<'help> {
/// # use clap::{App, Arg, ArgSettings};
/// Arg::new("file")
/// .long("file")
/// .setting(ArgSettings::TakesValue)
/// .setting(ArgSettings::AllowEmptyValues)
/// .forbid_empty_values(true)
/// # ;
/// ```
/// The default is to *not* allow empty values.
///
/// The default is allowing empty values.
///
/// ```rust
/// # use clap::{App, Arg, ErrorKind, ArgSettings};
/// let res = App::new("prog")
/// .arg(Arg::new("cfg")
/// .long("config")
/// .short('v')
/// .setting(ArgSettings::TakesValue))
/// .takes_value(true))
/// .try_get_matches_from(vec![
/// "prog", "--config="
/// ]);
///
/// assert!(res.is_err());
/// assert_eq!(res.unwrap_err().kind, ErrorKind::EmptyValue);
/// assert!(res.is_ok());
/// assert_eq!(res.unwrap().value_of("config"), None);
/// ```
/// By adding this setting, we can allow empty values
///
/// By adding this setting, we can forbid empty values.
///
/// ```rust
/// # use clap::{App, Arg, ArgSettings};
/// # use clap::{App, Arg, ErrorKind, ArgSettings};
/// let res = App::new("prog")
/// .arg(Arg::new("cfg")
/// .long("config")
/// .short('v')
/// .setting(ArgSettings::TakesValue)
/// .setting(ArgSettings::AllowEmptyValues))
/// .takes_value(true)
/// .forbid_empty_values(true))
/// .try_get_matches_from(vec![
/// "prog", "--config="
/// ]);
///
/// assert!(res.is_ok());
/// assert_eq!(res.unwrap().value_of("config"), None);
/// assert!(res.is_err());
/// assert_eq!(res.unwrap_err().kind, ErrorKind::EmptyValue);
/// ```
/// [`ArgSettings::TakesValue`]: ./enum.ArgSettings.html#variant.TakesValue
#[inline]
pub fn forbid_empty_values(self, multi: bool) -> Self {
if multi {
self.setting(ArgSettings::NoEmptyValues)
} else {
self.unset_setting(ArgSettings::NoEmptyValues)
}
}

/// Placeholder documentation
#[inline]
pub fn multiple_values(self, multi: bool) -> Self {
if multi {
self.setting(ArgSettings::MultipleValues)
Expand Down
12 changes: 6 additions & 6 deletions src/build/arg/settings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ bitflags! {
struct Flags: u32 {
const REQUIRED = 1;
const MULTIPLE_OCC = 1 << 1;
const EMPTY_VALS = 1 << 2;
const NO_EMPTY_VALS = 1 << 2;
const GLOBAL = 1 << 3;
const HIDDEN = 1 << 4;
const TAKES_VAL = 1 << 5;
Expand Down Expand Up @@ -39,7 +39,7 @@ impl_settings! { ArgSettings, ArgFlags,
Required("required") => Flags::REQUIRED,
MultipleOccurrences("multipleoccurrences") => Flags::MULTIPLE_OCC,
MultipleValues("multiplevalues") => Flags::MULTIPLE_VALS,
AllowEmptyValues("allowemptyvalues") => Flags::EMPTY_VALS,
NoEmptyValues("noemptyvalues") => Flags::NO_EMPTY_VALS,
Hidden("hidden") => Flags::HIDDEN,
TakesValue("takesvalue") => Flags::TAKES_VAL,
UseValueDelimiter("usevaluedelimiter") => Flags::USE_DELIM,
Expand Down Expand Up @@ -80,8 +80,8 @@ pub enum ArgSettings {
MultipleValues,
/// Allows an arg to appear multiple times
MultipleOccurrences,
/// Allows an arg accept empty values such as `""`
AllowEmptyValues,
/// Disallows an arg from accepting empty values such as `""`
NoEmptyValues,
/// Hides an arg from the help message
Hidden,
/// Allows an argument to take a value (such as `--option value`)
Expand Down Expand Up @@ -130,8 +130,8 @@ mod test {
ArgSettings::AllowHyphenValues
);
assert_eq!(
"allowemptyvalues".parse::<ArgSettings>().unwrap(),
ArgSettings::AllowEmptyValues
"noemptyvalues".parse::<ArgSettings>().unwrap(),
ArgSettings::NoEmptyValues
);
assert_eq!(
"hidepossiblevalues".parse::<ArgSettings>().unwrap(),
Expand Down
1 change: 1 addition & 0 deletions src/parse/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ pub enum ErrorKind {
/// let res = App::new("prog")
/// .arg(Arg::new("color")
/// .setting(ArgSettings::TakesValue)
/// .setting(ArgSettings::NoEmptyValues)
/// .long("color"))
/// .try_get_matches_from(vec!["prog", "--color="]);
/// assert!(res.is_err());
Expand Down
4 changes: 2 additions & 2 deletions src/parse/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1153,15 +1153,15 @@ impl<'help, 'app> Parser<'help, 'app> {
// has_eq: --flag=value
let mut has_eq = false;
let no_val = val.is_none();
let empty_vals = opt.is_set(ArgSettings::AllowEmptyValues);
let no_empty_vals = opt.is_set(ArgSettings::NoEmptyValues);
let min_vals_zero = opt.min_vals.unwrap_or(1) == 0;
let require_equals = opt.is_set(ArgSettings::RequireEquals);

debug!("Parser::parse_opt; Checking for val...");
if let Some(fv) = val {
has_eq = fv.starts_with("=");
let v = fv.trim_start_n_matches(1, b'=');
if !empty_vals && (v.is_empty() || (require_equals && !has_eq)) {
if no_empty_vals && (v.is_empty() || (require_equals && !has_eq)) {
debug!("Found Empty - Error");
return Err(ClapError::empty_value(
opt,
Expand Down
4 changes: 1 addition & 3 deletions src/parse/validator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,9 +125,7 @@ impl<'help, 'app, 'parser> Validator<'help, 'app, 'parser> {
));
}
}
if !arg.is_set(ArgSettings::AllowEmptyValues)
&& val.is_empty()
&& matcher.contains(&arg.id)
if arg.is_set(ArgSettings::NoEmptyValues) && val.is_empty() && matcher.contains(&arg.id)
{
debug!("Validator::validate_arg_values: illegal empty val found");
return Err(Error::empty_value(
Expand Down
6 changes: 5 additions & 1 deletion tests/default_vals.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,11 @@ fn opts() {
#[test]
fn opt_without_value_fail() {
let r = App::new("df")
.arg(Arg::from("-o [opt] 'some opt'").default_value("default"))
.arg(
Arg::from("-o [opt] 'some opt'")
.default_value("default")
.forbid_empty_values(true),
)
.try_get_matches_from(vec!["", "-o"]);
assert!(r.is_err());
let err = r.unwrap_err();
Expand Down
129 changes: 129 additions & 0 deletions tests/empty_values.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
mod utils;

use clap::{App, Arg, ArgSettings, ErrorKind};

#[test]
fn empty_values() {
let m = App::new("config")
.arg(Arg::new("config").long("config").takes_value(true))
.get_matches_from(&["config", "--config", ""]);
assert_eq!(m.value_of("config"), Some(""));
}

#[test]
fn empty_values_with_equals() {
let m = App::new("config")
.arg(Arg::new("config").long("config").takes_value(true))
.get_matches_from(&["config", "--config="]);
assert_eq!(m.value_of("config"), Some(""));

let m = App::new("config")
.arg(Arg::new("config").short('c').takes_value(true))
.get_matches_from(&["config", "-c="]);
assert_eq!(m.value_of("config"), Some(""))
}

#[test]
fn no_empty_values() {
let m = App::new("config")
.arg(
Arg::new("config")
.long("config")
.takes_value(true)
.forbid_empty_values(true),
)
.try_get_matches_from(&["config", "--config", ""]);
assert!(m.is_err());
assert_eq!(m.unwrap_err().kind, ErrorKind::EmptyValue);

let m = App::new("config")
.arg(
Arg::new("config")
.short('c')
.takes_value(true)
.forbid_empty_values(true),
)
.try_get_matches_from(&["config", "-c", ""]);
assert!(m.is_err());
assert_eq!(m.unwrap_err().kind, ErrorKind::EmptyValue)
}

#[test]
fn no_empty_values_with_equals() {
let m = App::new("config")
.arg(
Arg::new("config")
.long("config")
.takes_value(true)
.setting(ArgSettings::NoEmptyValues),
)
.try_get_matches_from(&["config", "--config="]);
assert!(m.is_err());
assert_eq!(m.unwrap_err().kind, ErrorKind::EmptyValue);

let m = App::new("config")
.arg(
Arg::new("config")
.short('c')
.takes_value(true)
.setting(ArgSettings::NoEmptyValues),
)
.try_get_matches_from(&["config", "-c="]);
assert!(m.is_err());
assert_eq!(m.unwrap_err().kind, ErrorKind::EmptyValue);
}

#[test]
fn no_empty_values_without_equals() {
let m = App::new("config")
.arg(
Arg::new("config")
.long("config")
.takes_value(true)
.setting(ArgSettings::NoEmptyValues),
)
.try_get_matches_from(&["config", "--config"]);
assert!(m.is_err());
assert_eq!(m.unwrap_err().kind, ErrorKind::EmptyValue);

let m = App::new("config")
.arg(
Arg::new("config")
.short('c')
.takes_value(true)
.setting(ArgSettings::NoEmptyValues),
)
.try_get_matches_from(&["config", "-c"]);
assert!(m.is_err());
assert_eq!(m.unwrap_err().kind, ErrorKind::EmptyValue)
}

#[test]
fn no_empty_values_without_equals_but_requires_equals() {
let app = App::new("config").arg(
Arg::new("config")
.long("config")
.takes_value(true)
.setting(ArgSettings::NoEmptyValues)
.setting(ArgSettings::RequireEquals),
);
let m = app.clone().try_get_matches_from(&["config", "--config"]);
// Should error on no equals rather than empty value.
assert!(m.is_err());
assert_eq!(m.unwrap_err().kind, ErrorKind::NoEquals);

static NO_EUQALS_ERROR: &str =
"error: Equal sign is needed when assigning values to '--config=<config>'.
USAGE:
config [OPTIONS]
For more information try --help";

assert!(utils::compare_output(
app,
"config --config",
NO_EUQALS_ERROR,
true
));
}
11 changes: 7 additions & 4 deletions tests/opts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ fn require_equals_fail() {
.arg(
Arg::new("cfg")
.setting(ArgSettings::RequireEquals)
.setting(ArgSettings::NoEmptyValues)
.setting(ArgSettings::TakesValue)
.long("config"),
)
Expand Down Expand Up @@ -104,6 +105,7 @@ fn require_equals_no_empty_values_fail() {
Arg::new("cfg")
.setting(ArgSettings::TakesValue)
.setting(ArgSettings::RequireEquals)
.setting(ArgSettings::NoEmptyValues)
.long("config"),
)
.arg(Arg::new("some"))
Expand All @@ -119,7 +121,6 @@ fn require_equals_empty_vals_pass() {
Arg::new("cfg")
.setting(ArgSettings::TakesValue)
.setting(ArgSettings::RequireEquals)
.setting(ArgSettings::AllowEmptyValues)
.long("config"),
)
.try_get_matches_from(vec!["prog", "--config="]);
Expand Down Expand Up @@ -422,8 +423,10 @@ fn did_you_mean() {
fn issue_665() {
let res = App::new("tester")
.arg("-v, --reroll-count=[N] 'Mark the patch series as PATCH vN'")
.arg(Arg::from(
"--subject-prefix [Subject-Prefix] 'Use [Subject-Prefix] instead of the standard [PATCH] prefix'") )
.arg(
Arg::from("--subject-prefix [Subject-Prefix] 'Use [Subject-Prefix] instead of the standard [PATCH] prefix'")
.setting(ArgSettings::NoEmptyValues)
)
.try_get_matches_from(vec!["test", "--subject-prefix", "-v", "2"]);

assert!(res.is_err());
Expand All @@ -449,7 +452,7 @@ fn issue_1047_min_zero_vals_default_val() {

fn issue_1105_setup(argv: Vec<&'static str>) -> Result<ArgMatches, clap::Error> {
App::new("opts")
.arg(Arg::from("-o, --option [opt] 'some option'").setting(ArgSettings::AllowEmptyValues))
.arg(Arg::from("-o, --option [opt] 'some option'"))
.arg(Arg::from("--flag 'some flag'"))
.try_get_matches_from(argv)
}
Expand Down

0 comments on commit a1d4bd8

Please sign in to comment.