-
-
Notifications
You must be signed in to change notification settings - Fork 223
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow extends
, macro
and import
inside an included template
#915
Conversation
What is the use case? Why do you want this? |
This allows me to use macros fully inside an included template without having to create another macro file that is associated with the included template and importing it using the same name everywhere I include this template. |
Nice! I'm working on something similar: allowing imports and macro declarations everywhere. So big 👍 for this from me. :) Do you have some numbers for the impact on compilation performance by any chance? |
No, I didn't measure the performance impact. Though I suspect it wouldn't get much worse because this caches included templates better, but it does clone the |
Let's see if there is a change already and if there one big enough, we can start optimizing (if @djc is ok with this feature of course). |
I don't know how to measure the performance impact, can you explain me? |
Very ad-hoc way to do it (if nothing comes out from this, I think it's mostly ok): write a file with like 10 templates of different sizes. Then run |
With big files full of |
Can you provide us the code you used to test so we can also confirm please? |
I used the following to generate the templates. from datetime import datetime
def generate_big_if(num: int, level: int = 0) -> str:
indent = " " * level
if num <= 1:
return f"{indent}hello {datetime.now()}\n"
right = num // 2
left = num - right
return (
indent
+ "{% if true %}\n"
+ generate_big_if(left, level + 1)
+ indent
+ "{% else %}\n"
+ generate_big_if(right, level + 1)
+ indent
+ "{% endif %}\n"
)
def main():
for i in range(9):
with open(f"templates/{i}.html", "w+") as f:
f.write(generate_big_if(int(1.75 ** (i + 3))))
if __name__ == "__main__":
main() And had this file as well: {% include "1.html" %}
{% include "2.html" %}
{% include "3.html" %}
{% include "4.html" %}
{% include "5.html" %}
{% include "6.html" %}
{% include "7.html" %}
{% include "8.html" %}
{% include "1.html" %}
{% include "2.html" %}
{% include "3.html" %}
{% include "4.html" %}
{% include "5.html" %}
{% include "6.html" %}
{% include "7.html" %}
{% include "8.html" %} And compiled: use askama::Template;
#[derive(Template)]
#[template(path = "0.html")]
struct T0;
#[derive(Template)]
#[template(path = "1.html")]
struct T1;
#[derive(Template)]
#[template(path = "2.html")]
struct T2;
#[derive(Template)]
#[template(path = "3.html")]
struct T3;
#[derive(Template)]
#[template(path = "4.html")]
struct T4;
#[derive(Template)]
#[template(path = "5.html")]
struct T5;
#[derive(Template)]
#[template(path = "6.html")]
struct T6;
#[derive(Template)]
#[template(path = "7.html")]
struct T7;
#[derive(Template)]
#[template(path = "8.html")]
struct T8;
#[derive(Template)]
#[template(path = "9.html")]
struct T9;
fn main() {
println!("Hello, world!");
} |
Thanks! Confirming the impact on my side. |
I have 1.97s before this PR and 1.98s after. I think the impact is indeed quite small. Well, let's see what @djc thinks about this now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I'm no longer concerned about performance.
let source = get_template_source(&path)?; | ||
check.push((path, source)); | ||
if !map.contains_key(&path) { | ||
map.insert(path.clone(), Parsed::default()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand the strategy of proactively inserting a fake entry into map
here. Why is that necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parsed
variable has to be owned and not mutably borrowed from map
because we are going to insert new entries into it. Inserting a dummy entry prevents re-adding the same file multiple times to check
before we process it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth adding a code comment to explain it then. ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I add it to the commit: "Improve performance of find_used_templates", or make a separate one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Signed-off-by: max <[email protected]>
34fdddb
to
f885dfb
Compare
Does it look good to you @djc? |
Ping @djc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, sorry for the long delay -- one more nit.
Please squash that change into the originating commit. |
Signed-off-by: max <[email protected]>
Signed-off-by: max <[email protected]>
Signed-off-by: max <[email protected]>
I squashed it |
🎉 |
Thanks for this! Is it going into a release soon? It's very useful with partials for htmx. |
I wrote up some thoughts on release planning in #1035. |
This makes
include
work more like in Jinja2 , and closes (#572).This also improves caching of included templates by storing them inside the
contexts
map.