Skip to content

Pass variables to sub-templates more reliably even if indirectly#397

Merged
GuillaumeGomez merged 5 commits intoaskama-rs:masterfrom
Kijewski:pr-fast-writable-values
Apr 17, 2025
Merged

Pass variables to sub-templates more reliably even if indirectly#397
GuillaumeGomez merged 5 commits intoaskama-rs:masterfrom
Kijewski:pr-fast-writable-values

Conversation

@Kijewski
Copy link
Copy Markdown
Member

No description provided.

@Kijewski
Copy link
Copy Markdown
Member Author

Since we are doing a breaking release, I think this would be a good addition. It changes the signature of askama::filters::FastWritable, which is public.

Comment thread askama/src/helpers.rs
Comment thread testing/tests/values.rs
@Kijewski Kijewski force-pushed the pr-fast-writable-values branch from f9000ef to 9895d48 Compare April 13, 2025 00:00
@Kijewski
Copy link
Copy Markdown
Member Author

I removed the filter |wordcount. It's not (easily) possible change the filter to have access to runtime values, which makes it a usage hazard. And actually, I cannot think of a good usecase for this filter.

@Kijewski Kijewski force-pushed the pr-fast-writable-values branch from 9895d48 to 69b6de4 Compare April 13, 2025 00:08
Comment thread askama/src/filters/alloc.rs
Comment thread askama/src/filters/alloc.rs
Comment thread askama/src/filters/alloc.rs
Comment thread askama/src/filters/humansize.rs Outdated
Ok(HtmlSafeOutput(Linebreaks(source)))
}

pub struct Linebreaks<S>(S);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Just to be sure: this commit is just about optimizing the rendering of the filters modified here, right?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sorry, I don't understand the question.

Comment thread askama/src/filters/alloc.rs
Comment thread askama/src/filters/alloc.rs
Comment thread askama/src/filters/alloc.rs Outdated
Comment thread askama/src/filters/alloc.rs
Comment thread book/src/filters.md Outdated
@GuillaumeGomez
Copy link
Copy Markdown
Collaborator

Wouldn't it make more sense to just provide the Values object to all filter call instead of doing it only for FastWritable?

@Kijewski Kijewski force-pushed the pr-fast-writable-values branch from ad7b930 to 51f9d24 Compare April 15, 2025 11:08
@Kijewski Kijewski force-pushed the pr-fast-writable-values branch 2 times, most recently from 94db84d to 8abe84d Compare April 15, 2025 11:22
@Kijewski
Copy link
Copy Markdown
Member Author

Wouldn't it make more sense to just provide the Values object to all filter call instead of doing it only for FastWritable?

I want custom filters to remain as simple to use as possible for the average case.

@GuillaumeGomez
Copy link
Copy Markdown
Collaborator

Wouldn't it make more sense to just provide the Values object to all filter call instead of doing it only for FastWritable?

I want custom filters to remain as simple to use as possible for the average case.

Is it though? Erf, It'd be nice if we could have a survey for this or something... I have a few thousands followers on socials so we can actually do it now that I think about it. What do you think?

If you agree, then we need to have a short explanation on the two possibilities:

  1. Pass Values to all filters
  2. If you want to have access to Values, you need to return a type which implements FastWritable

@Kijewski
Copy link
Copy Markdown
Member Author

Kijewski commented Apr 15, 2025

Sure, a poll would be great!

@Kijewski
Copy link
Copy Markdown
Member Author

Thinking again about it, no, simply passing the runtime values as a filter argument would solve nothing: The runtime values need to be passed on to the filter source, if it can accept this argument, and we still want to be compatible with any fmt::Display input.

@GuillaumeGomez
Copy link
Copy Markdown
Collaborator

The idea I had was to force all filters to have a Values argument. Not sure why it wouldn't be compatible with fmt::Display though?

@Kijewski
Copy link
Copy Markdown
Member Author

Kijewski commented Apr 15, 2025

I meant, even if we provide the argument to custom filters, the rest of the changes are still needed. I'm happy to review a different solution, though, I could not come up with another solution. :)

The only problem I see with always providing the value is that we lose one register if the filter is not #[inline]. But, the optimizer would hopefully take care of that.

Comment thread book/src/filters.md
### Runtime values
[#runtime-values]: #runtime-values

It is possible to access [runtime values](./runtime.html) in custom filters:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

So if Values is always provided, I suppose we need to update this part, right?

Also, might be nice to add docs about FastWritable. Gonna open an issue for it.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

But an example would still be nice for using Values in filters!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This example is "advanced" (not sure how to word it better). Since now filters all receive the Values argument, they just need to make use of it, no need to return a type implementing FastWritable. "Simpler is better" yada yada, you know the drill. 🤣

However, it'd be nice to also add a chapter in the book about FastWritable (hence why I opened #402).

Copy link
Copy Markdown
Member Author

@Kijewski Kijewski Apr 16, 2025

Choose a reason for hiding this comment

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

The example already is simplified, without special return types, etc. Have a look, maybe you didn't see the changes in my last force-push. :)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Oh indeed. Sorry about that. ^^'

@GuillaumeGomez GuillaumeGomez merged commit 2c1e86e into askama-rs:master Apr 17, 2025
41 checks passed
@Kijewski Kijewski deleted the pr-fast-writable-values branch April 17, 2025 09:54
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.

3 participants