From 8a2e9804d7896c0c239d7c6058b73cece69f8cb5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Benjamin=20K=C3=A4stner?= <benjamin.kaestner@gmail.com>
Date: Fri, 14 Aug 2020 17:27:58 +0200
Subject: [PATCH 1/3] Provide tests for Arg::default_value_if

While reading the code for the yaml translation, I've noticed that
there is a bug in the macro `yaml_opt_str` as well as a wrong `null`
value in the test fixture.

These tests add the expected behaviour on the given fixture, e.g.

    prog
and
    prog <WRONG>
where `<WRONG>` is **not** `other` should yield `None` for
`"positional2"`, whereas

    prog other
should yield "something" and

    prog --flag ARBITRARY_VALUE
should yield "some". The first two tests

- default_value_if_not_triggered
- default_value_if_not_triggered_by_argument

fail, as the second positional arguments *gets set*, although its
conditions aren't fulfilled.
---
 tests/yaml.rs | 45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

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");
+}

From 2b101ad7e5617466d60267f88a2295a83533f0df Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Benjamin=20K=C3=A4stner?= <benjamin.kaestner@gmail.com>
Date: Fri, 14 Aug 2020 17:41:40 +0200
Subject: [PATCH 2/3] Properly handle YAML null macros for default_value_if

The macro `yaml_opt_str` is only used in `yaml_tuple3`, which again is
only used for `default_value_if`. Unfortunately, the current test
doesn't make sense, as a v.is_null() indicates a Yaml::Null, on wich
`as_str()` always returns `None`. Instead, the condition should be
negated, as the documentation of `default_value_if` hints:

> **NOTE:** If using YAML the values should be laid out as
> follows (`None` can be represented as `null` in YAML)

The case `$v.is_null()` should therefore lead to `None`, whereas all
other cases should be interpreted `as_str()`.
---
 src/build/macros.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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)),

From 15c441708b26f43914a26a77f004e21147b75605 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Benjamin=20K=C3=A4stner?= <benjamin.kaestner@gmail.com>
Date: Fri, 14 Aug 2020 17:46:51 +0200
Subject: [PATCH 3/3] Fix YAML null value in fixture

The YAML null value is called `null` in YAML, not `Null`. yaml-rust
handles those values according to spec, so we should use the correct
capitalization.

See https://yaml.org/spec/1.2/spec.html#id2803362 for more
information.
---
 tests/fixtures/app.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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