Repository: cobalt-org/liquid-rust
Pull request #140 (2017-10-27 09:54:50)

feat(value): Add null value
This adds the missing null type to the values of liquid, thus allowing to
parse serde_json and others containing null-values in the data directly

epage: My hope has been to avoid the [billion dollar mistake](https://en.wikipedia.org/wiki/Tony_Hoare#Apologies_and_retractions).

Is there a use case for you to have null in your data?
gnunicorn: Well, this is for parsing data in cobalt, where any json can contain a `null` value and serde currently refuses to load the data into value then -- giving you rather obscure errors.

I don't think we are repeating that case here but are downwards compatible to those formats. We aren't repeating that problem, as all values have a way to be checked for being empty and will before interacting with it and `null`.

I see your point that this might not be something we want in liquid though and therefore should be handled inside cobalt (where I'd argue, we should be able to parse any valid JSON and `null` is a valid JSON value). If you agree on that we might want to discuss how to do that though: can we extend the serde-parsing into `Value` from cobalt? Then we could just use `false`. But I'd need some hints on how to achieve that.
epage: What is the reason for having null in your cobalt data?  Is it because you are loading a existing data source and don't plan to touch the null part of the data?  Do you need to touch the null part and need liquid to handle it gracefully?

> I see your point that this might not be something we want in liquid though and therefore should be handled inside cobalt (where I'd argue, we should be able to parse any valid JSON and null is a valid JSON value). 

Really, the question is whether we are loading arbitrary json/yaml/toml or loading liquid values.   My initial impression is to load liquid Values unless someone has a reasonable use case that can't be supported.
epage: > for AreWebYet I am just downloading crates.io JSON-dumps of packages and put them into the data folder. Those can and do contain null values (e.g. https://github.com/bashyHQ/arewewebyet/blob/gh-pages/_data/pkgs/afterparty.json )

Thanks for the use case.  I am fine moving forward with this.
gnunicorn: @epage it's green.
epage: Thanks for making the changes!
gnunicorn: @epage don't be too happy to soon. I pushed before fixing the commit logs because the tests is going mad on me and wanted your input on it:

```
running 2 tests
test serialize_nil ... ok
test deserialize_nil ... FAILED

failures:

---- deserialize_nil stdout ----
	thread 'deserialize_nil' panicked at 'assertion failed: `(left == right)`
  left: `Nil`,
 right: `Nil`', tests/value.rs:79:4
note: Run with `RUST_BACKTRACE=1` for a backtrace.


failures:
    deserialize_nil

test result: FAILED. 1 passed; 1 failed; 0 ignored; 0 measured; 10 filtered out

```

Any ideas?
epage: I wonder if `actual` is a `liquid::Str`.

Patch:
diff --git a/src/filters.rs b/src/filters.rs
index fcaedad..b17f6cd 100644
--- a/src/filters.rs
+++ b/src/filters.rs
@@ -849,6 +849,7 @@ pub fn default(input: &Value, args: &[Value]) -> FilterResult {
         Array(ref a) => a.is_empty(),
         Bool(b) => !b,
         Num(_) => false,
+        Value::Nil => true,
     };
 
     if use_default {
diff --git a/src/value.rs b/src/value.rs
index ce1c0cf..f92e858 100644
--- a/src/value.rs
+++ b/src/value.rs
@@ -16,6 +16,7 @@ pub enum Value {
     Str(String),
     Array(Array),
     Object(Object),
+    Nil,
 }
 
 /// Type representing a Liquid array, payload of the `Value::Array` variant
@@ -132,6 +133,7 @@ impl PartialEq<Value> for Value {
             (&Value::Str(ref x), &Value::Str(ref y)) => x == y,
             (&Value::Array(ref x), &Value::Array(ref y)) => x == y,
             (&Value::Object(ref x), &Value::Object(ref y)) => x == y,
+            (&Value::Nil, &Value::Nil) => true,
 
             // encode Ruby truthiness; all values except false and nil
             // are true, and we don't have a notion of nil
@@ -164,6 +166,7 @@ impl ToString for Value {
             Value::Num(ref x) => x.to_string(),
             Value::Bool(ref x) => x.to_string(),
             Value::Str(ref x) => x.to_owned(),
+            Value::Nil => "".to_owned(),
             Value::Array(ref x) => {
                 let arr: Vec<String> = x.iter().map(|v| v.to_string()).collect();
                 arr.join(", ")
@@ -269,6 +272,11 @@ mod test {
         assert_eq!(TRUE, Value::Num(0f32));
     }
 
+    #[test]
+    fn nil_equality() {
+        assert_eq!(Value::Nil, Value::Nil);
+    }
+
     #[test]
     fn object_equality() {
         let mut values = HashMap::<String, Value>::new();


Test patch:
diff --git a/tests/value.rs b/tests/value.rs
index 858755a..235bcd8 100644
--- a/tests/value.rs
+++ b/tests/value.rs
@@ -66,6 +66,22 @@ pub fn deserialize_bool() {
     assert_eq!(actual, liquid::Value::Bool(false));
 }
 
+#[test]
+pub fn serialize_nil() {
+    let actual = liquid::Value::Nil;
+    let actual = serde_yaml::to_string(&actual).unwrap();
+    assert_diff!(&actual, "---\n~", "", 0);
+}
+
+#[test]
+pub fn deserialize_nil() {
+    let actual: liquid::Value = serde_yaml::from_str("---\n~").unwrap();
+    assert_eq!(actual, liquid::Value::Nil);
+
+    let actual: liquid::Value = serde_yaml::from_str("---\n- ").unwrap();
+    assert_eq!(actual, liquid::Value::Array(vec![liquid::Value::Nil]));
+}
+
 #[test]
 pub fn serialize_str() {
     let actual = liquid::Value::str("Hello");