feat(value): Add null value #140

gnunicorn · 2017-10-27T09:54:50Z

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 · 2017-10-27T11:37:24Z

My hope has been to avoid the billion dollar mistake.

Is there a use case for you to have null in your data?

gnunicorn · 2017-10-27T11:58:23Z

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 · 2017-10-27T12:54:33Z

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 · 2017-10-27T16:04:31Z

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 · 2017-10-30T18:48:50Z

@epage it's green.

epage

Sorry, I know I said we just needed green. If you want, this can go in now and we can open issues on these comments.

epage · 2017-10-30T19:12:31Z

src/value.rs

@@ -164,6 +165,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::Null => "null".to_owned(),


Sorry, just thought about this last night. Are we aligned with Shopify on this?

Does it matter?

I am not sure, what we use the ToString for within templates. From the Shopify docs it is rather vague how this would come to be used. In their cheat sheet they only explain Nil to be falsy and all other of their filters and loops seem to make a check for truthy-ness before executing, which probably stems from that being the implied case in ruby. And is already a behavior we don't have that way (which we discussed the other day).

I am also happy to make it "", which is probably closer to the interpretation of their underlying ruby implementation...

You're link answered the question of what ToString should do

Tags or outputs that return nil will not print anything to the page.

https://help.shopify.com/themes/liquid/basics/types#nil

btw if Shopify calls it Nil then we probably should too (Value::Nil).

epage · 2017-10-30T19:13:54Z

src/value.rs

@@ -16,6 +16,7 @@ pub enum Value {
    Str(String),
    Array(Array),
    Object(Object),
+    Null,


I wonder how serde decides that None gets serialized as this.

We should add a serde test for this
https://github.com/cobalt-org/liquid-rust/blob/master/tests/value.rs

I honestly have no idea. I don't really get the inner-workings of serde -- but it is on my bucket list :D .

I used serde_json as the base and it defined Null in their value and in JSON there also is no None but only null. For TOML and YAML I am not even sure a nothing-type even exists, I think it is just represented as empty strings ...

epage · 2017-10-30T21:11:11Z

Thanks for making the changes!

gnunicorn · 2017-10-30T21:12:31Z

@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 · 2017-10-30T21:15:17Z

I wonder if actual is a liquid::Str.

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

epage · 2017-10-30T21:56:33Z

I assume you'd like me to publish liquid and cobalt with this as soon as possible?

* **syntax:** Add `arr[0]` and `obj["name"]` indexing (PR #141, fixes #127) * **value:** Add nil value to support foreign data (PR #140)

gnunicorn force-pushed the add-null-value branch 2 times, most recently from 52de3ef to 7cda923 Compare October 30, 2017 17:14

epage reviewed Oct 30, 2017

View reviewed changes

feat(value): Add nil value

89f6660

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

gnunicorn force-pushed the add-null-value branch from 3973c0b to 89f6660 Compare October 30, 2017 21:28

epage merged commit bf67d97 into cobalt-org:master Oct 30, 2017

epage added a commit that referenced this pull request Nov 8, 2017

chore: Release 0.11

742896f

* **syntax:** Add `arr[0]` and `obj["name"]` indexing (PR #141, fixes #127) * **value:** Add nil value to support foreign data (PR #140)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(value): Add null value #140

feat(value): Add null value #140

gnunicorn commented Oct 27, 2017

epage commented Oct 27, 2017

gnunicorn commented Oct 27, 2017

epage commented Oct 27, 2017

epage commented Oct 27, 2017

gnunicorn commented Oct 30, 2017

epage left a comment

epage Oct 30, 2017

gnunicorn Oct 30, 2017

epage Oct 30, 2017

epage Oct 30, 2017

gnunicorn Oct 30, 2017

epage commented Oct 30, 2017

gnunicorn commented Oct 30, 2017

epage commented Oct 30, 2017

epage commented Oct 30, 2017

feat(value): Add null value #140

feat(value): Add null value #140

Conversation

gnunicorn commented Oct 27, 2017

epage commented Oct 27, 2017

gnunicorn commented Oct 27, 2017

epage commented Oct 27, 2017

epage commented Oct 27, 2017

gnunicorn commented Oct 30, 2017

epage left a comment

Choose a reason for hiding this comment

epage Oct 30, 2017

Choose a reason for hiding this comment

gnunicorn Oct 30, 2017

Choose a reason for hiding this comment

epage Oct 30, 2017

Choose a reason for hiding this comment

epage Oct 30, 2017

Choose a reason for hiding this comment

gnunicorn Oct 30, 2017

Choose a reason for hiding this comment

epage commented Oct 30, 2017

gnunicorn commented Oct 30, 2017

epage commented Oct 30, 2017

epage commented Oct 30, 2017