Skip to content
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

Fix nested templates block #1029

Merged
merged 4 commits into from
May 16, 2024
Merged

Conversation

GuillaumeGomez
Copy link
Contributor

@GuillaumeGomez GuillaumeGomez commented May 2, 2024

Fixes #1022.

Problem was that we were still rendering the content into the WritableBuffer.

@GuillaumeGomez GuillaumeGomez force-pushed the nested-templates-block branch 2 times, most recently from ac82e67 to ae96c56 Compare May 2, 2024 15:48
@GuillaumeGomez
Copy link
Contributor Author

And fixed clippy lints.

@GuillaumeGomez GuillaumeGomez changed the title Nested templates block Fix nested templates block May 2, 2024
@@ -11,7 +11,7 @@ struct FragmentSimple<'a> {
fn test_fragment_simple() {
let simple = FragmentSimple { name: "world" };

assert_eq!(simple.render().unwrap(), "\n\n<p>Hello world!</p>\n");
assert_eq!(simple.render().unwrap(), "\n<p>Hello world!</p>");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For all lines removal, I'm not sure whether it was bad before and this PR fixed it as a side effect or the opposite...

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this particular case, IMO the previous state was good and this PR breaks it.

{% block body %}
<p>Hello {{ name }}!</p>
{% endblock %}

Arguably this should render newlines before and after the p node?

@@ -1813,6 +1817,7 @@ struct Buffer {
indent: u8,
// Whether the output buffer is currently at the start of a line
start: bool,
discard: bool,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the previous PR added discard in WritableBuffer. Remind me why we have both and why we need to discard in both (instead of discarding in one of them only)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One is the actual string where we push content and the other is pending AST that wasn't pushed yet into the string.

So in this case, if we only want to render a specific named block, which is then followed by another one (which we're not interested into), both will get rendered. However, the block we want to render could also be in other blocks, so we still need to go through the whole tree of each present block, but only render the block that was specified.

#[derive(Template)]
#[template(path = "blocks.txt", block = "section")]
struct Section<'a> {
values: Vec<&'a str>,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: why couldn't this just be a slice?


#[test]
fn test_specific_block() {
let values: Vec<&str> = vec!["a", "b", "c"];
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: pretty sure this doesn't need the type annotation.

@@ -11,7 +11,7 @@ struct FragmentSimple<'a> {
fn test_fragment_simple() {
let simple = FragmentSimple { name: "world" };

assert_eq!(simple.render().unwrap(), "\n\n<p>Hello world!</p>\n");
assert_eq!(simple.render().unwrap(), "\n<p>Hello world!</p>");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this particular case, IMO the previous state was good and this PR breaks it.

{% block body %}
<p>Hello {{ name }}!</p>
{% endblock %}

Arguably this should render newlines before and after the p node?

testing/tests/inheritance.rs Show resolved Hide resolved
@GuillaumeGomez
Copy link
Contributor Author

Mystery solved (for the missing trailing backline characters): I accidentaly removed child.flush_ws(def.ws2);. Much less diff in the tests now. \o/

@rellfy
Copy link

rellfy commented May 3, 2024

I can confirm this resolved an issue I had where contents from the template leaked into the block-fragmented child template. I added another test case here: #1032

@djc djc mentioned this pull request May 7, 2024
7 tasks
@djc djc merged commit 4d237ab into djc:main May 16, 2024
16 checks passed
@GuillaumeGomez GuillaumeGomez deleted the nested-templates-block branch May 16, 2024 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compilation error when using nested template blocks with in a single template file
4 participants