Repository: clap-rs/clap
Pull request #2072 (2020-08-14 16:05:25)

Fix YAML's default_value_if handling
While working on another issue I was completely puzzled by the `yaml_opt_str!`, as it immediately starts with `$v.is_null()` and then uses `as_str()` on a `YAML::Null` value. While trying to get to the root of the issue, I've noticed that this bug will only manifest itself in `default_value_if` when compiled with `yaml` support.

I've added some integration tests for the YAML part and also noticed that `null` was misspelled in the fixture, which kept the bug hidden. If you don't apply `e20cc1c` from the set of commits, you'll notice that *all* `yaml` tests break down due to `[flag, null, some]`.

**Please note that this is my first non-documentation Rust pull request ever and I'm happy about any feedback and criticism.**

I wasn't sure about the commit message style, as the recent commits didn't use the style mentioned in the contributing guide. I've tried to emulate the style from the recent commits. Also wasn't sure whether you want to have a single squashed commit, or three atomic changes/additions, but I'll happily change the messages/squash the commits if necessary.

bors[bot]: :clock1: Waiting for PR status (Github check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set.
pksunkara: > I wasn't sure about the commit message style, as the recent commits didn't use the style mentioned in the contributing guide. I've tried to emulate the style from the recent commits.

Yeah, we haven't been following conventional commit style since we are working on v3. We will probably decide on this after v3 is released.

> Also wasn't sure whether you want to have a single squashed commit, or three atomic changes/additions, but I'll happily change the messages/squash the commits if necessary.

Your commits are pretty good. They each deal with different stuff and your messages are self-explanatory. I think it's okay to have them like. I find that it's always a balancing issue.

But if you have some tests failing, you will probably need to fix that tests by amending the relevant commit.


Patch:
diff --git a/src/build/macros.rs b/src/build/macros.rs
index 5840d668..899840ab 100644
--- a/src/build/macros.rs
+++ b/src/build/macros.rs
@@ -65,7 +65,7 @@ macro_rules! yaml_vec_or_str {
 #[cfg(feature = "yaml")]
 macro_rules! yaml_opt_str {
     ($v:expr) => {{
-        if $v.is_null() {
+        if !$v.is_null() {
             Some(
                 $v.as_str()
                     .unwrap_or_else(|| panic!("failed to convert YAML {:?} value to a string", $v)),


Test patch:
diff --git a/tests/fixtures/app.yaml b/tests/fixtures/app.yaml
index 86b59552..b8f2988e 100644
--- a/tests/fixtures/app.yaml
+++ b/tests/fixtures/app.yaml
@@ -21,7 +21,7 @@ args:
         about: tests positionals with exclusions
         index: 2
         default_value_if:
-            - [flag, Null, some]
+            - [flag, null, some]
             - [positional, other, something]
     - flag:
         short: f
diff --git a/tests/yaml.rs b/tests/yaml.rs
index 12b3affd..c18039d3 100644
--- a/tests/yaml.rs
+++ b/tests/yaml.rs
@@ -54,3 +54,48 @@ fn value_hint() {
         .unwrap();
     assert_eq!(arg.get_value_hint(), ValueHint::FilePath);
 }
+
+#[test]
+fn default_value_if_not_triggered_by_argument() {
+    let yml = load_yaml!("fixtures/app.yaml");
+    let app = App::from(yml);
+
+    // Fixtures use "other" as value
+    let matches = app.try_get_matches_from(vec!["prog", "wrong"]).unwrap();
+
+    assert!(matches.value_of("positional2").is_none());
+}
+
+#[test]
+fn default_value_if_triggered_by_matching_argument() {
+    let yml = load_yaml!("fixtures/app.yaml");
+    let app = App::from(yml);
+
+    let matches = app.try_get_matches_from(vec!["prog", "other"]).unwrap();
+    assert_eq!(matches.value_of("positional2").unwrap(), "something");
+}
+
+#[test]
+fn default_value_if_triggered_by_flag() {
+    let yml = load_yaml!("fixtures/app.yaml");
+    let app = App::from(yml);
+
+    let matches = app
+        .try_get_matches_from(vec!["prog", "--flag", "flagvalue"])
+        .unwrap();
+
+    assert_eq!(matches.value_of("positional2").unwrap(), "some");
+}
+
+#[test]
+fn default_value_if_triggered_by_flag_and_argument() {
+    let yml = load_yaml!("fixtures/app.yaml");
+    let app = App::from(yml);
+
+    let matches = app
+        .try_get_matches_from(vec!["prog", "--flag", "flagvalue", "other"])
+        .unwrap();
+
+    // First condition triggers, therefore "some"
+    assert_eq!(matches.value_of("positional2").unwrap(), "some");
+}