Repository: djc/askama
Pull request #517 (2021-07-18 09:47:40)

Implement {{loop.cycle(…)}} similar to Jinja

djc: When this was discussed before, I proposed we should make this `loop.cycle([1, 2, 3])` to make this more like Rust (where we don't have arbitrary numbers of arguments. What do you think?
Kijewski: Sorry, I didn't see the prior discussions, I only saw that is was mentioned in #427. The braces it does look more like rust. The only problem I foresee is that someone might try to use the function like `loop.cycle(self.array)`. What code would you generate in this case?

```rs
{
    // This moves the variables out of self:
    let _cycle = self.array;
    _cycle[_cycle.len() % index]
}

{
    // If the variable is used for printing, then this should be fine,
    // but it's used in e.g. an addition, it fails:
    let _cycle = &self.array;
    _cycle[_cycle.len() % index]
}

{
    // This must not be used for e.g. {{ loop.cycle([a(), b(), c()]) }},
    // but I guess you shouldn't use the function like that in any case:
    self.sequence[self.array.len() % index]
}
```
djc: We take a reference to the array and index into it?
vallentin: I was just wondering, what would happen if you moved `loop.cycle` into a macro, and called it multiple times in the loop.

Jinja:

```jinja
{% macro foo() %}
{{ loop.cycle("A", "B", "C") }}
{% endmacro %}

{% for v in values %}
{{ foo() }} {{ foo() }} {{ foo() }}
{% endfor %}
```

I was wondering if it would output `AAABBBCCC...` or `ABCABCABC...`. I did a quick test using 2 online "Jinja live parsers". The first one froze, the second one said "'loop' is undefined".

Should Askama support this? If yes, should the output be the former or the latter?


vallentin: I only spent a couple of minutes searching, but I wasn't able to find a specific source stating the behavior, thus why I did a quick test. Both behaviors are possible using Jinja's `cycler()`, so if `loop.cycle()` is a shorthand for creating a `cycler(...)` before the loop, and then in the loop doing `.next()`. Then the output would be `ABCABCABC...` :)

i.e., if the previous Jinja code (using `loop.cycle("A", "B", "C")`) is a shorthand of this:

```jinja
{% set abc = cycler("A", "B", "C") %}

{% macro foo() %}
    {{ abc.next() }}
{% endmacro %}

{% for v in values %}
    {{ foo() }} {{ foo() }} {{ foo() }}
{% endfor %}
```

djc: Oh, I didn't know that Jinja had that feature. Still, since we're specifically referring to the loop context here, I think we should stick with the semantics mentioned in my previous comment.
vallentin: > Still, since we're specifically referring to the loop context here

I agree, with "given loop context". I was just posing the question, so we're all on the same page :)
Kijewski: I'm not a 100% sure if I like the variant with brackets better, so I didn't squash the commit immediately. 
djc: I think the variant with brackets is nice, especially if that means it can accept arbitrary slices.

Would you mind rebasing this (and ideally squashing it)?

Patch:
diff --git a/askama_shared/src/generator.rs b/askama_shared/src/generator.rs
index f6d7cca..7500cc7 100644
--- a/askama_shared/src/generator.rs
+++ b/askama_shared/src/generator.rs
@@ -1278,15 +1278,38 @@ impl<'a, S: std::hash::BuildHasher> Generator<'a, S> {
         method: &str,
         args: &[Expr<'_>],
     ) -> Result<DisplayWrap, CompileError> {
-        if let Expr::Var("self") = obj {
-            buf.write("self");
+        if matches!(obj, Expr::Var("loop")) {
+            match method {
+                "cycle" => match args {
+                    [arg] => {
+                        if matches!(arg, Expr::Array(arr) if arr.is_empty()) {
+                            panic!("loop.cycle(…) cannot use an empty array.");
+                        }
+                        buf.write("({");
+                        buf.write("let _cycle = &(");
+                        self.visit_expr(buf, arg)?;
+                        buf.writeln(");")?;
+                        buf.writeln("let _len = _cycle.len();")?;
+                        buf.writeln("if _len == 0 {")?;
+                        buf.writeln("return ::core::result::Result::Err(::askama::Error::Fmt(::core::fmt::Error));")?;
+                        buf.writeln("}")?;
+                        buf.writeln("_cycle[_loop_item.index % _len]")?;
+                        buf.writeln("})")?;
+                    }
+                    _ => return Err("loop.cycle(…) expects exactly one argument".into()),
+                },
+                s => return Err(format!("unknown loop method: {:?}", s).into()),
+            }
         } else {
-            self.visit_expr(buf, obj)?;
+            if let Expr::Var("self") = obj {
+                buf.write("self");
+            } else {
+                self.visit_expr(buf, obj)?;
+            }
+            buf.write(&format!(".{}(", normalize_identifier(method)));
+            self._visit_args(buf, args)?;
+            buf.write(")");
         }
-
-        buf.write(&format!(".{}(", normalize_identifier(method)));
-        self._visit_args(buf, args)?;
-        buf.write(")");
         Ok(DisplayWrap::Unwrapped)
     }
 


Test patch:
diff --git a/testing/tests/loops.rs b/testing/tests/loops.rs
index 49c37e0..f014068 100644
--- a/testing/tests/loops.rs
+++ b/testing/tests/loops.rs
@@ -372,3 +372,48 @@ fn test_loop_break_continue() {
     };
     assert_eq!(t.render().unwrap(), "x1yx2yx3yx11x4yx5y");
 }
+
+#[derive(Template)]
+#[template(
+    source = r#"{% for v in values %}{{loop.cycle(["r", "g", "b"])}}{{v}},{% endfor %}"#,
+    ext = "txt"
+)]
+struct ForCycle<'a> {
+    values: &'a [u8],
+}
+
+#[test]
+fn test_for_cycle() {
+    let t = ForCycle {
+        values: &[1, 2, 3, 4, 5, 6, 7, 8, 9],
+    };
+    assert_eq!(t.render().unwrap(), "r1,g2,b3,r4,g5,b6,r7,g8,b9,");
+}
+
+#[derive(Template)]
+#[template(
+    source = r#"{% for v in values %}{{loop.cycle(cycle)}}{{v}},{% endfor %}"#,
+    ext = "txt"
+)]
+struct ForCycleDynamic<'a> {
+    values: &'a [u8],
+    cycle: &'a [char],
+}
+
+#[test]
+fn test_for_cycle_dynamic() {
+    let t = ForCycleDynamic {
+        values: &[1, 2, 3, 4, 5, 6, 7, 8, 9],
+        cycle: &['a', 'b', 'c', 'd'],
+    };
+    assert_eq!(t.render().unwrap(), "a1,b2,c3,d4,a5,b6,c7,d8,a9,");
+}
+
+#[test]
+fn test_for_cycle_empty() {
+    let t = ForCycleDynamic {
+        values: &[1, 2, 3, 4, 5, 6, 7, 8, 9],
+        cycle: &[],
+    };
+    assert!(t.render().is_err())
+}
diff --git a/testing/tests/ui/loop_cycle_wrong_argument_count.rs b/testing/tests/ui/loop_cycle_wrong_argument_count.rs
new file mode 100644
index 0000000..e87f4f4
--- /dev/null
+++ b/testing/tests/ui/loop_cycle_wrong_argument_count.rs
@@ -0,0 +1,13 @@
+use askama::Template;
+
+#[derive(Template)]
+#[template(
+    source = r#"{% for v in values %}{{ loop.cycle("r", "g", "b") }}{{ v }},{% endfor %}"#,
+    ext = "txt"
+)]
+struct ForCycle<'a> {
+    values: &'a [u8],
+}
+
+fn main() {
+}
diff --git a/testing/tests/ui/loop_cycle_wrong_argument_count.stderr b/testing/tests/ui/loop_cycle_wrong_argument_count.stderr
new file mode 100644
index 0000000..acf3bb1
--- /dev/null
+++ b/testing/tests/ui/loop_cycle_wrong_argument_count.stderr
@@ -0,0 +1,7 @@
+error: loop.cycle(…) expects exactly one argument
+ --> $DIR/loop_cycle_wrong_argument_count.rs:3:10
+  |
+3 | #[derive(Template)]
+  |          ^^^^^^^^
+  |
+  = note: this error originates in the derive macro `Template` (in Nightly builds, run with -Z macro-backtrace for more info)