Repository: cobalt-org/liquid-rust
Pull request #438 (2021-03-20 02:20:51)

feat(lib): Implement Jekyll's version of sort
Part of #311.  Continuation of #546

epage: @limeschool : posting this to keep you updated.
limeschool: Thanks for working on this, @epage! If I was actually capable of writing decent code, I would've made a PR myself.

Apologies if I came off as whiny earlier, and thanks, really.
epage: Want to finish this?  I'm trying to wrap up another project before I focus on liquid/cobalt (so I can better focus).

The main other thing to know is the function [try_find](https://docs.rs/liquid-core/0.22.0/liquid_core/model/fn.try_find.html)
limeschool: I'm working on other things right now myself, but I will give it a looking at once I learn how to write tests (I'm really amateur, haha) and once I've looked at how everything works in this crate.
epage: >  but I will give it a looking at once I learn how to write tests (I'm really amateur, haha) 

The PR includes some of Jekyll's tests being ported with Rust with the remaining ones partially ported and commented out.  Comparing [the original tests](https://github.com/jekyll/jekyll/blob/master/test/test_filters.rb#L1365) with the ones that I already ported should get you most of the way there.

As for the other parts, the surrounding glue is written, just the implementation needs writing.  As a hint`

```
let args = self.args.evaluate(runtime)?;
```
takes `SortArgs` and constructs a derive-constructed struct that has `property: Option<str>, nils: Option<str>`.  So you can just do `args.property` and `args.nils`.
emmyoh: @epage I have something like this:
```rust
fn safe_property_getter<'a>(value: &'a Value, property: String) -> Value {
    value
        .as_object()
        .and_then(move |obj| {
            if let Some(scalar) = obj.as_scalar() {
                try_find(&property.clone(), &vec![scalar]).map(|v| v.clone().to_value())
            } else {
                obj.get(&property).map(|v| v.to_value())
            }
        })
        .unwrap_or(Value::Nil)
}
```
It can get a top-level property, but not anything deeper (ie, it fails when `property` is of the form `XXX.YYY`).
Have you figured out how to get nested properties of an `ObjectView`?
emmyoh: Down to two failing tests:
- `raise_exception_when_input_is_nil`
- `return_sorted_by_property_array_with_nils_first`

If you have the time, would you mind taking a look?

Patch:
diff --git a/crates/core/src/parser/parser.rs b/crates/core/src/parser/parser.rs
index 92335d8..92f7ddb 100644
--- a/crates/core/src/parser/parser.rs
+++ b/crates/core/src/parser/parser.rs
@@ -78,6 +78,16 @@ pub fn parse(text: &str, options: &Language) -> Result<Vec<Box<dyn Renderable>>>
     Ok(renderables)
 }
 
+/// Given a `Variable` as a string, parses it into a `Variable`.
+pub fn parse_variable(text: &str) -> Result<Variable> {
+    let variable = LiquidParser::parse(Rule::Variable, text)
+        .map_err(convert_pest_error)?
+        .next()
+        .expect("Parsing a variable failed.");
+
+    Ok(parse_variable_pair(variable))
+}
+
 /// Parses a `Scalar` from a `Pair` with a literal value.
 /// This `Pair` must be `Rule::Literal`.
 fn parse_literal(literal: Pair) -> Value {
@@ -124,7 +134,7 @@ fn parse_literal(literal: Pair) -> Value {
 
 /// Parses a `Variable` from a `Pair` with a variable.
 /// This `Pair` must be `Rule::Variable`.
-fn parse_variable(variable: Pair) -> Variable {
+fn parse_variable_pair(variable: Pair) -> Variable {
     if variable.as_rule() != Rule::Variable {
         panic!("Expected variable.");
     }
@@ -163,7 +173,7 @@ fn parse_value(value: Pair) -> Expression {
 
     match value.as_rule() {
         Rule::Literal => Expression::Literal(parse_literal(value)),
-        Rule::Variable => Expression::Variable(parse_variable(value)),
+        Rule::Variable => Expression::Variable(parse_variable_pair(value)),
         _ => unreachable!(),
     }
 }
@@ -971,7 +981,7 @@ impl<'a> TagToken<'a> {
     /// Tries to obtain a `Variable` from this token.
     pub fn expect_variable(mut self) -> TryMatchToken<'a, Variable> {
         match self.unwrap_variable() {
-            Ok(t) => TryMatchToken::Matches(parse_variable(t)),
+            Ok(t) => TryMatchToken::Matches(parse_variable_pair(t)),
             Err(_) => {
                 self.expected.push(Rule::Variable);
                 TryMatchToken::Fails(self)
@@ -1121,7 +1131,7 @@ mod test {
     }
 
     #[test]
-    fn test_parse_variable() {
+    fn test_parse_variable_pair() {
         let variable = LiquidParser::parse(Rule::Variable, "foo[0].bar.baz[foo.bar]")
             .unwrap()
             .next()
@@ -1137,7 +1147,7 @@ mod test {
         let mut expected = Variable::with_literal("foo");
         expected.extend(indexes);
 
-        assert_eq!(parse_variable(variable), expected);
+        assert_eq!(parse_variable_pair(variable), expected);
     }
 
     #[test]
diff --git a/crates/lib/src/jekyll/array.rs b/crates/lib/src/jekyll/array.rs
index a7ddc24..d9808e6 100644
--- a/crates/lib/src/jekyll/array.rs
+++ b/crates/lib/src/jekyll/array.rs
@@ -1,8 +1,14 @@
+use std::cmp;
 use std::fmt::Write;
 
+use liquid_core::model::try_find;
+use liquid_core::model::KStringCow;
+use liquid_core::model::ValueViewCmp;
+use liquid_core::parser::parse_variable;
 use liquid_core::Expression;
 use liquid_core::Result;
 use liquid_core::Runtime;
+use liquid_core::ValueCow;
 use liquid_core::{
     Display_filter, Filter, FilterParameters, FilterReflection, FromFilterParameters, ParseFilter,
 };
@@ -10,6 +16,127 @@ use liquid_core::{Value, ValueView};
 
 use crate::invalid_input;
 
+#[derive(Debug, Default, FilterParameters)]
+struct SortArgs {
+    #[parameter(description = "The property accessed by the filter.", arg_type = "str")]
+    property: Option<Expression>,
+    #[parameter(
+        description = "nils appear before or after non-nil values, either ('first' | 'last')",
+        arg_type = "str"
+    )]
+    nils: Option<Expression>,
+}
+
+#[derive(Clone, ParseFilter, FilterReflection)]
+#[filter(
+    name = "sort",
+    description = "Sorts items in an array. The order of the sorted array is case-sensitive.",
+    parameters(SortArgs),
+    parsed(SortFilter)
+)]
+pub struct Sort;
+
+#[derive(Debug, Default, FromFilterParameters, Display_filter)]
+#[name = "sort"]
+struct SortFilter {
+    #[parameters]
+    args: SortArgs,
+}
+
+#[derive(Copy, Clone)]
+enum NilsOrder {
+    First,
+    Last,
+}
+
+fn safe_property_getter<'v>(
+    value: &'v Value,
+    property: &KStringCow,
+    runtime: &dyn Runtime,
+) -> ValueCow<'v> {
+    let variable = parse_variable(property).expect("Failed to parse variable");
+    if let Some(path) = variable.try_evaluate(runtime) {
+        try_find(value, path.as_slice()).unwrap_or(ValueCow::Borrowed(&Value::Nil))
+    } else {
+        ValueCow::Borrowed(&Value::Nil)
+    }
+}
+
+fn nil_safe_compare(
+    a: &dyn ValueView,
+    b: &dyn ValueView,
+    nils: NilsOrder,
+) -> Option<cmp::Ordering> {
+    if a.is_nil() && b.is_nil() {
+        Some(cmp::Ordering::Equal)
+    } else if a.is_nil() {
+        match nils {
+            NilsOrder::First => Some(cmp::Ordering::Less),
+            NilsOrder::Last => Some(cmp::Ordering::Greater),
+        }
+    } else if b.is_nil() {
+        match nils {
+            NilsOrder::First => Some(cmp::Ordering::Greater),
+            NilsOrder::Last => Some(cmp::Ordering::Less),
+        }
+    } else {
+        ValueViewCmp::new(a).partial_cmp(&ValueViewCmp::new(b))
+    }
+}
+
+fn as_sequence<'k>(input: &'k dyn ValueView) -> Box<dyn Iterator<Item = &'k dyn ValueView> + 'k> {
+    if let Some(array) = input.as_array() {
+        array.values()
+    } else if input.is_nil() {
+        Box::new(vec![].into_iter())
+    } else {
+        Box::new(std::iter::once(input))
+    }
+}
+
+impl Filter for SortFilter {
+    fn evaluate(&self, input: &dyn ValueView, runtime: &dyn Runtime) -> Result<Value> {
+        let args = self.args.evaluate(runtime)?;
+
+        let input: Vec<_> = as_sequence(input).collect();
+        if input.is_empty() {
+            return Err(invalid_input("Non-empty array expected"));
+        }
+        if args.property.is_some() && !input.iter().all(|v| v.is_object()) {
+            return Err(invalid_input("Array of objects expected"));
+        }
+        let nils = if let Some(nils) = &args.nils {
+            match nils.to_kstr().as_str() {
+                "first" => NilsOrder::First,
+                "last" => NilsOrder::Last,
+                _ => {
+                    return Err(invalid_input(
+                        "Invalid nils order. Must be \"first\" or \"last\".",
+                    ))
+                }
+            }
+        } else {
+            NilsOrder::First
+        };
+
+        let mut sorted: Vec<Value> = input.iter().map(|v| v.to_value()).collect();
+        if let Some(property) = &args.property {
+            // Using unwrap is ok since all of the elements are objects
+            sorted.sort_by(|a, b| {
+                nil_safe_compare(
+                    safe_property_getter(a, property, runtime).as_view(),
+                    safe_property_getter(b, property, runtime).as_view(),
+                    nils,
+                )
+                .unwrap_or(cmp::Ordering::Equal)
+            });
+        } else {
+            sorted.sort_by(|a, b| nil_safe_compare(a, b, nils).unwrap_or(cmp::Ordering::Equal));
+        }
+        Ok(Value::array(sorted))
+    }
+}
+
 #[derive(Debug, FilterParameters)]
 struct PushArgs {
     #[parameter(description = "The element to append to the array.")]
@@ -195,6 +322,16 @@ impl Filter for ArrayToSentenceStringFilter {
 mod tests {
     use super::*;
 
+    #[test]
+    fn unit_sort() {
+        let input = &liquid_core::value!(["Z", "b", "c", "a"]);
+        let desired_result = liquid_core::value!(["Z", "a", "b", "c"]);
+        assert_eq!(
+            liquid_core::call_filter!(Sort, input).unwrap(),
+            desired_result
+        );
+    }
+
     #[test]
     fn unit_push() {
         let input = liquid_core::value!(["Seattle", "Tacoma"]);


Test patch:
diff --git a/crates/lib/tests/conformance_jekyll/mod.rs b/crates/lib/tests/conformance_jekyll/mod.rs
index 9f82948..6bfc2f0 100644
--- a/crates/lib/tests/conformance_jekyll/mod.rs
+++ b/crates/lib/tests/conformance_jekyll/mod.rs
@@ -1,1 +1,2 @@
 mod slugify_test;
+mod test_filters;
diff --git a/crates/lib/tests/conformance_jekyll/test_filters.rs b/crates/lib/tests/conformance_jekyll/test_filters.rs
new file mode 100644
index 0000000..9587c02
--- /dev/null
+++ b/crates/lib/tests/conformance_jekyll/test_filters.rs
@@ -0,0 +1,142 @@
+use liquid_lib::jekyll;
+
+mod sort_filter {
+    use super::*;
+
+    #[test]
+    fn raise_exception_when_input_is_nil() {
+        let input = liquid_core::Value::Nil;
+        assert!(liquid_core::call_filter!(jekyll::Sort, input).is_err());
+    }
+    #[test]
+    fn return_sorted_numbers() {
+        assert_eq!(
+            v!([1, 2, 2.2, 3]),
+            liquid_core::call_filter!(jekyll::Sort, v!([3, 2.2, 2, 1])).unwrap()
+        );
+    }
+
+    #[test]
+    fn return_sorted_strings() {
+        assert_eq!(
+            v!(["10", "2"]),
+            liquid_core::call_filter!(jekyll::Sort, v!(["10", "2"])).unwrap()
+        );
+        assert_eq!(
+            v!(["FOO", "Foo", "foo"]),
+            liquid_core::call_filter!(jekyll::Sort, v!(["foo", "Foo", "FOO"])).unwrap()
+        );
+        assert_eq!(
+            v!(["_foo", "foo", "foo_"]),
+            liquid_core::call_filter!(jekyll::Sort, v!(["foo_", "_foo", "foo"])).unwrap()
+        );
+        // Cyrillic
+        assert_eq!(
+            v!(["ВУЗ", "Вуз", "вуз"]),
+            liquid_core::call_filter!(jekyll::Sort, v!(["Вуз", "вуз", "ВУЗ"])).unwrap()
+        );
+        assert_eq!(
+            v!(["_вуз", "вуз", "вуз_"]),
+            liquid_core::call_filter!(jekyll::Sort, v!(["вуз_", "_вуз", "вуз"])).unwrap()
+        );
+        // Hebrew
+        assert_eq!(
+            v!(["אלף", "בית"]),
+            liquid_core::call_filter!(jekyll::Sort, v!(["בית", "אלף"])).unwrap()
+        );
+    }
+
+    #[test]
+    fn return_sorted_by_property_array() {
+        assert_eq!(
+            liquid_core::value!([{ "a": 1 }, { "a": 2 }, { "a": 3 }, { "a": 4 }]),
+            liquid_core::call_filter!(
+                jekyll::Sort,
+                liquid_core::value!([{ "a": 4 }, { "a": 3 }, { "a": 1 }, { "a": 2 }]),
+                "a"
+            )
+            .unwrap()
+        );
+    }
+
+    #[test]
+    fn return_sorted_by_property_array_with_numeric_strings_sorted_as_numbers() {
+        assert_eq!(
+            liquid_core::value!([{ "a": ".5" }, { "a": "0.65" }, { "a": "10" }]),
+            liquid_core::call_filter!(
+                jekyll::Sort,
+                liquid_core::value!([{ "a": "10" }, { "a": ".5" }, { "a": "0.65" }]),
+                "a"
+            )
+            .unwrap(),
+        );
+    }
+
+    #[test]
+    fn return_sorted_by_property_array_with_numeric_strings_first() {
+        assert_eq!(
+            liquid_core::value!([{ "a": ".5" }, { "a": "0.6" }, { "a": "twelve" }]),
+            liquid_core::call_filter!(
+                jekyll::Sort,
+                liquid_core::value!([{ "a": "twelve" }, { "a": ".5" }, { "a": "0.6" }]),
+                "a"
+            )
+            .unwrap()
+        );
+    }
+
+    #[test]
+    fn return_sorted_by_property_array_with_numbers_and_strings() {
+        assert_eq!(
+            liquid_core::value!([{ "a": "1" }, { "a": "1abc" }, { "a": "20" }]),
+            liquid_core::call_filter!(
+                jekyll::Sort,
+                liquid_core::value!([{ "a": "20" }, { "a": "1" }, { "a": "1abc" }]),
+                "a"
+            )
+            .unwrap()
+        );
+    }
+
+    #[test]
+    fn return_sorted_by_property_array_with_nils_first() {
+        let ary = liquid_core::value!([{ "a": 2 }, { "b": 1 }, { "a": 1 }]);
+        assert_eq!(
+            liquid_core::value!([{ "b": 1 }, { "a": 1 }, { "a": 2 }]),
+            liquid_core::call_filter!(jekyll::Sort, ary, "a").unwrap()
+        );
+        assert_eq!(
+            liquid_core::value!([{ "b": 1 }, { "a": 1 }, { "a": 2 }]),
+            liquid_core::call_filter!(jekyll::Sort, ary, "a", "first").unwrap()
+        );
+    }
+
+    #[test]
+    fn return_sorted_by_property_array_with_nils_last() {
+        assert_eq!(
+            liquid_core::value!([{ "a": 1 }, { "a": 2 }, { "b": 1 }]),
+            liquid_core::call_filter!(
+                jekyll::Sort,
+                liquid_core::value!([{ "a": 2 }, { "b": 1 }, { "a": 1 }]),
+                "a",
+                "last"
+            )
+            .unwrap()
+        );
+    }
+
+    #[test]
+    fn return_sorted_by_subproperty_array() {
+        assert_eq!(
+            liquid_core::value!([{ "a": { "b": 1 } }, { "a": { "b": 2 } },
+                    { "a": { "b": 3 } },]),
+            liquid_core::call_filter!(
+                jekyll::Sort,
+                liquid_core::value!([{ "a": { "b": 2 } }, { "a": { "b": 1 } },
+                                 { "a": { "b": 3 } },]),
+                "a.b"
+            )
+            .unwrap()
+        );
+    }
+}