Add support for importing template files with macros #51

larros · 2017-09-24T16:03:08Z

This is a try at an implementation of #40.

I've intentionally kept this as limited as possible because I'm not sure what functionality that should be included. Compared to Jinja there's no support for "import as" or "from import", instead all macros are added to the same namespace.

I first tried to implement this in generator.rs as some combination of the include and macro functionality but I couldn't get that past the borrow checker. The macros need to exist for the full generation and because they contain slices of the imported sources the imports need to be parsed before the generation starts. In the end I put the ownership of the imported sources and nodes in lib.rs in the same way as for the parent template.

djc

Very nice! Thanks for working on this. Please review my comments and let me know what you think.

djc · 2017-09-24T19:44:49Z

askama_shared/src/generator.rs

@@ -12,8 +12,8 @@ use std::collections::{HashMap, HashSet};
 use syn;


-pub fn generate(input: &TemplateInput, nodes: &[Node]) -> String {
-    Generator::default().build(&State::new(input, nodes))
+pub fn generate(input: &TemplateInput, nodes: &[Node], imported_nodes: &[Node]) -> String {


Nit: how do you feel about just calling this imported? imported_notes feels a little unwieldy.

Sounds good, but I will also change it to containing the macros directly.

djc · 2017-09-24T19:48:13Z

askama_shared/src/input.rs

@@ -129,6 +131,34 @@ impl<'a> TemplateMeta<'a> {
    }
 }

+pub struct Imports<'a> {


Given the way this is used, and the fact it relies on the parser, I think putting it in input is probably not quite right. Can you move it to lib.rs, between build_template() and the errors module?

djc · 2017-09-24T19:53:03Z

askama_shared/src/input.rs

+}
+
+impl <'a> Imports<'a> {
+    pub fn new(parent_nodes: &'a [Node], parent_path: &'a Path) -> Imports<'a> {


So we have three stages here:

Imports::new() finds import statements and gets the template sources for them

Imports::parse() turns the sources into Vec<Node>s

generator::State::new() finds the Node::Macros and puts them into the macro map

Is there any reason not do all in one place, so something like a get_imported_macros() that takes parent_nodes and parent_path and returns a MacroMap?

Upon reflection, I guess the template sources have to be preserved, since the AST nodes depend on them. Still, I think we can extract the Macro nodes earlier?

The first step is needed for ownership reasons but I think the other two can be combined. Then Imports::parse() would return the macros directly.

djc · 2017-09-24T20:12:03Z

testing/templates/import.html

@@ -0,0 +1,2 @@
+{% import "macro.html" -%}
+{%- call thrice(s) %}


Nit: please add a trailing newline here.

djc · 2017-09-24T20:12:34Z

testing/tests/import.rs

@@ -0,0 +1,16 @@
+#[macro_use]


Nit: please just append this test case to macro.rs.

larros · 2017-09-25T17:09:47Z

I've tried to fix all comments now.

I have one thing that needs to be decided on before completing this.
If namespaces for imported macros should be forced, like they are in Jinja, that should be added now to not cause a backwards incompatible change later.

djc · 2017-09-25T18:44:35Z

Looks good! Yes, I don't want to release this without forcing namespacing, but I think it would be nice to merge this into master now so that we can then take the next step separately. Okay?

larros · 2017-09-25T19:12:08Z

That's fine with me.
Do you have any ideas of the namespacing syntax? Jinja's "import as" feels very pythonic to me so maybe it's not the correct solution here.

djc · 2017-09-25T19:36:06Z

My policy has been to stay close to Jinja's syntax except where it does not make sense because there's no alignment with Rust. We can either do import as here, or maybe use as would be more Rusty. Do you have any alternatives in mind? @anowell / @Eijebong, any opinions?

larros · 2017-09-25T20:07:49Z

Now when I think more about this I don't think this is a place where it's worth it to divert from Jinja. There is still a need to express the same intent so just swapping words seems like a bad idea.

Eijebong · 2017-09-25T20:17:35Z

What about doing the same thing as tera ?

{% import "menu" as menu %}
{{ menu::app_menu_item(...) }}

djc · 2017-09-25T20:23:11Z

Does tera do something magic with extensions? I definitely wouldn't want that. Using :: instead of . might make sense though, and would actually make parsing easier.

Eijebong · 2017-09-25T20:25:42Z

Yeah, I don't like the magic extension thingy either (I'm not sure if it's even needed anymore), I just copy/pasted it from one of my templates. I like the :: though, it feels rusty :p

larros · 2017-09-25T20:27:47Z

I like ::.

djc · 2017-09-26T08:16:31Z

Looks like we have consensus on the scoped import syntax. I merged the base part already, since it seems to provide a good basis already.

djc · 2017-10-14T08:27:17Z

@larros are you still up for finishing the implementation of the ::-based scoped syntax sometime soon? If not, that's totally fine too -- I can probably just complete it.

larros · 2017-10-14T11:08:45Z

I have some time tomorrow so I can make a try.

jmafc · 2020-03-24T02:59:29Z

Was the "import as" syntax shown above, with the :: scoped calling ever implemented? I have a very similar example.

{% import "navigation.html" as navigation -%}
...
{{ navigation.secmenu() }}

If I try to build, I get error[E0609]: no field navigation on type .... If I replace the dot by ::, the message is error: proc-macro derive panicked ... help: message: unable to parse template. If I comment the second line, it builds but of course without the menu.
If it's still not implemented, is there some workaround? I have five import files, each with multiple macros.

djc · 2020-03-24T14:21:55Z

@jmafc look at the code from one of the test cases:

{%- import "macro.html" as scope -%}

{% call scope::thrice(s) %}

In other words, it should be :: rather than ., but macros are also called with {% call %} rather than as a simple function expression (exactly because the parser wouldn't be able to distinguish between a Rust function call and the macro invocation).

jmafc · 2020-03-24T14:23:43Z

@djc Thanks

Add support for importing template files with macros

609b03f

djc reviewed Sep 24, 2017

View reviewed changes

Fix review comments for macro imports

a592f82

djc merged commit df58dcb into djc:master Sep 26, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for importing template files with macros #51

Add support for importing template files with macros #51

larros commented Sep 24, 2017

djc left a comment

djc Sep 24, 2017

larros Sep 25, 2017

djc Sep 24, 2017

djc Sep 24, 2017

djc Sep 24, 2017

larros Sep 25, 2017

djc Sep 24, 2017

djc Sep 24, 2017

larros commented Sep 25, 2017

djc commented Sep 25, 2017

larros commented Sep 25, 2017

djc commented Sep 25, 2017

larros commented Sep 25, 2017

Eijebong commented Sep 25, 2017 •

edited

djc commented Sep 25, 2017

Eijebong commented Sep 25, 2017

larros commented Sep 25, 2017

djc commented Sep 26, 2017

djc commented Oct 14, 2017

larros commented Oct 14, 2017

jmafc commented Mar 24, 2020

djc commented Mar 24, 2020

jmafc commented Mar 24, 2020

		@@ -0,0 +1,2 @@
		{% import "macro.html" -%}
		{%- call thrice(s) %}

Add support for importing template files with macros #51

Add support for importing template files with macros #51

Conversation

larros commented Sep 24, 2017

djc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larros commented Sep 25, 2017

djc commented Sep 25, 2017

larros commented Sep 25, 2017

djc commented Sep 25, 2017

larros commented Sep 25, 2017

Eijebong commented Sep 25, 2017 • edited

djc commented Sep 25, 2017

Eijebong commented Sep 25, 2017

larros commented Sep 25, 2017

djc commented Sep 26, 2017

djc commented Oct 14, 2017

larros commented Oct 14, 2017

jmafc commented Mar 24, 2020

djc commented Mar 24, 2020

jmafc commented Mar 24, 2020

Eijebong commented Sep 25, 2017 •

edited