Repository: Keats/tera
Pull request #60 (2016-09-28 21:45:09)

Implement Include Tag
Simple {% include... %} tag implementation using the same context as the parent. Hope this is ok to merge.

Keats: Thanks for that!

I was thinking that it might be better to only have macros, see the last comment in https://github.com/Keats/tera/issues/19 for the reasoning.

I do find it hard when writing partial templates to know what are the variables available and macros solve that problem nicely, the only annoying bit being having to load the macro file.

What do you think?
andrelmartins: In my mind the two are orthogonal. I would like "macros" to enable
component reuse (especially for context variable rename) and would like
"include" so that I can split the main page in chunks for AJAX style
updates.

On Thu, Sep 29, 2016 at 10:34 AM, Vincent Prouillet <
notifications@github.com> wrote:

> Thanks for that!
>
> I was thinking that it might be better to only have macros, see the last
> comment in #19 <https://github.com/Keats/tera/issues/19> for the
> reasoning.
>
> I do find it hard when writing partial templates to know what are the
> variables available and macros solve that problem nicely, the only annoying
> bit being having to load the macro file.
>
> What do you think?
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/Keats/tera/pull/60#issuecomment-250417740>, or mute
> the thread
> <https://github.com/notifications/unsubscribe-auth/AExZ9Hg-rWMSRbMHCgjuS2j9KT2F-NZjks5qu4Y6gaJpZM4KJWbs>
> .
>

Keats: Do you have an example for the "split the main page in chunks for AJAX style updates."? I'm not entirely sure what that means.

I typically use include for (let's take something i'm working on right now, a static website) things like header, footer, `<head>` to keep it clean but they could all be macros and be cleaner.

@SergioBenitez ping since you seem to be using Tera as well, what's your take on that?
andrelmartins: An example would be a table with pagination. The javascript for the "next
page" button would issue an AJAX request to the server to re-render the
table with the new information. I don't want to re-render the entire page,
only the specific table, so I would split that portion into a separate
template file and use include from the main content template. Hope this
makes sense.

SergioBenitez: It seems that the `include` tag is a strict subset of the `macro` tag. If that's the case, then I would recommend implementing `macro` first, and then have `include` simply be an alias to `macro`.

Personally, I like `include`'s succinctness and would like it to be part of Tera. This particular PR needs a bit of work, in my opinion, and I would rather see this implemented in terms of `macro`.
andrelmartins: My only concern with implementing the include tag as sugar over macro is
that I still want to be able to render the target template independently
when I need to. That is, the template implementing the macro would still be
a valid target for "tera.render(...)". This is not necessarily true for
macros using the syntax shown in of
https://github.com/pallets/website/tree/master/templates/macros ,
especially if there is more than one macro per implementation file.



On Fri, Sep 30, 2016 at 9:13 PM, Sergio Benitez <notifications@github.com>
wrote:

> It seems that the include tag is a strict subset of the macro tag. If
> that's the case, then I would recommend implementing macro first, and
> then have include simply be an alias to macro.
>
> Personally, I like include's succinctness and would like it to be part of
> Tera. This particular PR needs a bit of work, in my opinion, and I would
> rather see this implemented in terms of macro.
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/Keats/tera/pull/60#issuecomment-250841101>, or mute
> the thread
> <https://github.com/notifications/unsubscribe-auth/AExZ9LXE4rgNYfMj6LQ48QWTaKICGS1jks5qvW1ZgaJpZM4KJWbs>
> .
>

SergioBenitez: Would a `tera.render_macro` method suffice, @andrelmartins?
andrelmartins: Yes, that would be fine. Although, at that point, it almost seems more work
to implement the "include" tag over the "macro" tag, than as an independent
tag :)

SergioBenitez: Well, I don't think so. `render_macro` will be needed by Tera anyways when macro is being rendered in a template. And the sugar for `include` will be handled at syntax level by changing it to simply be `macro .`.
andrelmartins: I guess so. Will the "include" tag also require a parameter list, like
regular macro invocations, or will it always pass the full context?

SergioBenitez: It should pass the current context.
andrelmartins: So, at parse time, when an "include" tag is found, the target file is
checked for macros (throwing an error if more than one macro is present,
otherwise we don't know which macro to invoke), and a macro Node is created
with the name of that macro. This could be a special macro node
(IncludeMacro) or a regular Macro node with some special flag. At render
time, the IncludeMacro or Macro+flag node would skip the regular macro
context creation and instead pass in the current context.

Is this how you imagine this working?

Keats: `render_macro` might not be needed in tera renderer as it should be able to use the renderer in `render.rs`, we will just have to build the context manually from the macro args.

I'm not sure the `include` being just sugar for macros would work as i think it's quite common to have several macros in a file (example from a Jinja2 project https://github.com/mrjoes/flask-admin/blob/master/flask_admin/templates/bootstrap3/admin/lib.html) and you can't just use the macro name as you can have several macros with the same name.

I'm tempted to have some kind of `include` like in that PR but not add any options to it (ie it always take the full context, if you want to rename stuff or change the context you have to use a macro)
SergioBenitez: How does having several macros with the same name work?

@andrelmartins I was thinking this would work as follows:

  1. Macro definitions would be discovered during template parsing and placed in a HashMap.
  2. `include` would just be rewritten to a `macro` use with the current context in the AST.
    * No lookup of any sort would happen here.
  3. During render time, the macro would be looked up in the HashMap and rendered.

@Keats I was suggesting a `render_macro` method to differentiate the namespaces.
Keats: You have to load macros your template, they are not loaded automatically.

Example:
```
{% import "utils2.html" as utils2 %}
{% from "utils.html" import macro_1 , macro_2 %}
// there you can call macro_1, macro_2 or utils.* and utils2 can have macros named macro_1 and macro_2 as well
```

Keats: One potential solution would be:

- add include first as an experimental feature in the way it's implemented in the PR
- add macro system: lots of choices to be made there I think, unless we copy exactly the Jinja2 ones
- try to retrofit the `include` into using macros and see if it's possible
SergioBenitez: @Keats That sounds good to me.

I'd still like to see an integration test or two added to the PR, especially a negative test where an `include` fails. @andrelmartins Can you do that?
Keats: The code looks ok to me.
As mentioned by @SergioBenitez, it's just missing some integrations test.
Add a test in `test/render_ok.rs` and one in `test/render_fail.rs` for success/failure and that's ready to merge!
andrelmartins: Ok, will do.

Keats: Thanks!
Keats: @andrelmartins Just needs a rebase and it's ready to go!
andrelmartins: Done.


Patch:
diff --git a/src/parser.rs b/src/parser.rs
index 9853ad4..5963177 100644
--- a/src/parser.rs
+++ b/src/parser.rs
@@ -35,6 +35,7 @@ pub enum Node {
     Raw(String),
     Extends(String),
     VariableBlock(Box<Node>),
+    Include(String),
 }
 
 impl Node {
@@ -135,6 +136,7 @@ impl_rdp! {
         block_start    = _{ variable_start | tag_start | comment_start }
 
         // Actual tags
+        include_tag     = !@{ tag_start ~ ["include"] ~ string ~ tag_end }
         extends_tag     = !@{ tag_start ~ ["extends"] ~ string ~ tag_end }
         variable_tag    = !@{ variable_start ~ expression ~ variable_end }
         comment_tag     = !@{ comment_start ~ (!comment_end ~ any )* ~ comment_end }
@@ -154,6 +156,7 @@ impl_rdp! {
         text       = { (!(block_start) ~ any )+ }
 
         content = @{
+            include_tag |
             variable_tag |
             comment_tag |
             block_tag ~ content* ~ endblock_tag |
@@ -204,6 +207,9 @@ impl_rdp! {
             (&head: text) => {
                 Ok(Some(Node::Text(head.to_string())))
             },
+            (_: include_tag, &name: string) => {
+                Ok(Some(Node::Include(name.replace("\"", "").to_string())))
+            },
             (_: variable_tag, exp: _expression()) => {
                 Ok(Some(Node::VariableBlock(Box::new(try!(exp)))))
             },
@@ -622,6 +628,13 @@ mod tests {
         assert!(parser.end());
     }
 
+    #[test]
+    fn test_include_tag() {
+        let mut parser = Rdp::new(StringInput::new("{% include \"component.html\" %}"));
+        assert!(parser.include_tag());
+        assert!(parser.end());
+    }
+
     #[test]
     fn test_comment_tag() {
         let mut parser = Rdp::new(StringInput::new("{# some text {{}} #}"));
diff --git a/src/render.rs b/src/render.rs
index aeb9167..a9800aa 100644
--- a/src/render.rs
+++ b/src/render.rs
@@ -346,6 +346,15 @@ impl<'a> Renderer<'a> {
 
     pub fn render_node(&mut self, node: Node) -> TeraResult<String> {
         match node {
+            Include(p) => {
+                let ast = try!(self.tera.get_template(&p)).ast.get_children();
+                let mut output = String::new();
+                for node in ast {
+                    output.push_str(&try!(self.render_node(node)));
+                }
+
+                Ok(output)
+            },
             Text(s) => Ok(s),
             Raw(s) => Ok(s.trim().to_string()),
             VariableBlock(exp) => self.render_variable_block(*exp),
@@ -412,6 +421,15 @@ mod tests {
         tera.render("hello", context)
     }
 
+    #[test]
+    fn test_render_include() {
+        let mut tera = Tera::default();
+        tera.add_template("world", "world");
+        tera.add_template("hello", "<h1>Hello {% include \"world\" %}</h1>");
+        let result = tera.render("hello", Context::new());
+        assert_eq!(result.unwrap(), "<h1>Hello world</h1>".to_owned());
+    }
+
     #[test]
     fn test_render_simple_string() {
         let result = render_template("<h1>Hello world</h1>", Context::new());


Test patch:
diff --git a/tests/expected/include.html b/tests/expected/include.html
new file mode 100755
index 0000000..5ae6416
--- /dev/null
+++ b/tests/expected/include.html
@@ -0,0 +1,17 @@
+<html>
+  <head>
+    <title>Products</title>
+  </head>
+  <body>
+    <div>
+      <table>
+        <thead>
+          <th>User</th><th>Number of reviews</th>
+        </thead>
+        <tr>
+          <td>bob</td><td>2</td>
+        </tr>
+      </table>
+    </div>
+  </body>
+</html>
diff --git a/tests/render-failures/inexisting_include.html b/tests/render-failures/inexisting_include.html
new file mode 100755
index 0000000..bbbb8bd
--- /dev/null
+++ b/tests/render-failures/inexisting_include.html
@@ -0,0 +1,1 @@
+{% include "hola" %}
diff --git a/tests/render_fails.rs b/tests/render_fails.rs
index 54d1959..cb15c97 100644
--- a/tests/render_fails.rs
+++ b/tests/render_fails.rs
@@ -59,3 +59,11 @@ fn test_error_render_iterate_non_array() {
     assert_eq!(result.is_err(), true);
     assert_eq!(result.unwrap_err().description(), "field is not an array".to_string());
 }
+
+#[test]
+fn test_error_render_include_inexistent() {
+    let result = render_tpl("inexisting_include.html");
+
+    assert_eq!(result.is_err(), true);
+    assert_eq!(result.unwrap_err().description(), "template not found".to_string());
+}
diff --git a/tests/render_ok.rs b/tests/render_ok.rs
index f2bb144..3d46bdd 100644
--- a/tests/render_ok.rs
+++ b/tests/render_ok.rs
@@ -109,3 +109,8 @@ fn test_ok_variable_tests() {
 fn test_ok_indexing() {
     assert_template_ok("tests/templates/indexing.html", None);
 }
+
+#[test]
+fn test_ok_include_template() {
+    assert_template_ok("tests/templates/include.html", Some("tests/templates/included.html"));
+}
diff --git a/tests/templates/include.html b/tests/templates/include.html
new file mode 100755
index 0000000..bb1e246
--- /dev/null
+++ b/tests/templates/include.html
@@ -0,0 +1,10 @@
+<html>
+  <head>
+    <title>Products</title>
+  </head>
+  <body>
+    <div>
+      {% include "included.html" %}
+    </div>
+  </body>
+</html>
diff --git a/tests/templates/included.html b/tests/templates/included.html
new file mode 100755
index 0000000..064cf02
--- /dev/null
+++ b/tests/templates/included.html
@@ -0,0 +1,8 @@
+<table>
+        <thead>
+          <th>User</th><th>Number of reviews</th>
+        </thead>
+        <tr>
+          <td>{{ username }}</td><td>{{ number_reviews }}</td>
+        </tr>
+      </table>
\ No newline at end of file