Add filters `|default`, `|assigned_or` and `|defined_or` #425

Kijewski · 2025-05-01T11:19:49Z

Also, enum Pluralize<S, P> is renamed into enum Either<L, R> and exported.

Cc @joshka.

Resolves #424.

Kijewski · 2025-05-01T11:25:18Z

I am not sure if DefaultUnwrappable is a good name.

Also, it would be possible to implement the trait for integers (testing if != 0), and Strings (testing if != "").

askama_derive/src/generator/filter.rs

GuillaumeGomez · 2025-05-01T15:14:17Z

Btw, no clue about a better for DefaultUnwrappable so I'm fine with it.

joshka · 2025-05-01T17:24:07Z

book/src/filters.md

+</li><li>
+
+```jinja
+{{ variable_or_expression | default(default_value, true) }}


This sort of parameter without a name makes it difficult to understand a template worth reading the template docs. I'd been inclined to either:

Replace a naked bool parameter with an enum that self documents (that may look verbose here), or

Replace a bool with a second named method. E.g. default_if_undefined might be a good name for the false branch. Alternatively default_if_none / err

I added a line that {{ variable_or_expression | default(default_value, boolean = true) }} works, too.

I'd still prefer explicit over implicit a bit. This is based on an assumption that the direction of askama is a jinja-like template library with natural rust idioms rather than a jinja compatible template library that adds rust conveniences. I'm not sure whether that assumption is correct however (intuitively I'd lean towards the former as being more useful)

As a single data point. Having not seen the jinja syntax for the default filter prior, even with the parameter named boolean in the CoPilot suggestion rather than the bare true, I still had no idea from reading it what that parameter meant or controlled seeing it for the first time. This suggests to me that it is poorly named in jinja, and something that we could make better pretty easily with good naming here.

joshka · 2025-05-01T18:35:31Z

Also, it would be possible to implement the trait for integers (testing if != 0), and Strings (testing if != "").

Empty string seems reasonable as that's rationally returned when a string is not entered somewhere, but most of the time a zero value seems like it would be only 0 if you're using that as a flag value, which is not very rusty, so I'd perhaps implement default_if_empty for strings (and str, cow perhaps), but not default_if_zero unless there's an explicit obvious use case where it makes sense.

Kijewski · 2025-05-01T22:27:52Z

Renamed trait to DefaultFilterable (I think that's better)
Implemented for Cow<'_, T> (I forgot about taat, good call!)
Implemented for Wrapping<T> & Saturating<T>
Implemented for NonZero* (infallably Ok(Some())) [just so you don't have to update the code if you use NonZeroU32 and u32]

I think implementing the type for integers is fine and useful. In many real-world cases you will find 0 being used as a poor-man's NULL. And unless boolean = true, it does not matter either way.

DefaultFilterable is not recursive: Some(0) returns 0; only the Option is evaluated, not the contained value.

joshka · 2025-05-02T00:32:36Z

askama/src/filters/builtin.rs

-pub fn pluralize<C, S, P>(count: C, singular: S, plural: P) -> Result<Pluralize<S, P>, C::Error>
+pub fn pluralize<C, S, P>(count: C, singular: S, plural: P) -> Result<Either<S, P>, C::Error>


Does this break semver (in a way that matters) for any consumers?
If so, would be worth keeping as Pluralize rather than version bumping the lib?

Well, version bumping will be needed since we plan to extend {% call %} blocks. So it's fine.

The type was unreachable before, so its name did not matter.

joshka · 2025-05-02T00:35:45Z

askama/src/filters/builtin.rs

+    fn as_filtered(&self) -> Result<Option<Self::Filtered<'_>>, Self::Error>;
+}
+
+const _: () = {


This isn't a syntax I've seen before, what's the rationale behind it?

To prevent impacting the upper scope.

In here I mostly use it like a #region. Every DefaultFilterable is in this scope and one click hides everything.

Got it. I know that often there's a tendency for larger module sizes in some Rust projects. This seems to be a symptom of that. I tend to address the same core problem (strongly related code that isn't in the outer scope) by using smaller narrower module (files) and liberal re-exports instead. This keeps the unit tests fairly close to the code it's testing, makes it easy to navigate and find code, etc. Obv. this is subjective, but maybe something to think about in future.

So a soft suggestion to extract this to filters/default.rs or something.

askama/src/filters/builtin.rs

joshka · 2025-05-02T01:01:55Z

book/src/filters.md

+</li><li>
+
+```jinja
+{{ variable_or_expression | default(default_value, true) }}


I'd still prefer explicit over implicit a bit. This is based on an assumption that the direction of askama is a jinja-like template library with natural rust idioms rather than a jinja compatible template library that adds rust conveniences. I'm not sure whether that assumption is correct however (intuitively I'd lean towards the former as being more useful)

As a single data point. Having not seen the jinja syntax for the default filter prior, even with the parameter named boolean in the CoPilot suggestion rather than the bare true, I still had no idea from reading it what that parameter meant or controlled seeing it for the first time. This suggests to me that it is poorly named in jinja, and something that we could make better pretty easily with good naming here.

askama_derive/src/generator/filter.rs

GuillaumeGomez · 2025-05-02T09:17:51Z

askama_derive/src/generator/filter.rs

+                default_value: None,
+            },
+            &FilterArgument {
+                name: "boolean",


Why not naming it "do_unwrap" or something along the line to better match what it's doing instead?

Question still open. :)

Kijewski · 2025-05-02T10:56:43Z

So, what do we want to do with the argument "boolean"? It's the name and semantics Jinja uses. The question is: How compatible do we want to be?

If we don't care to be 100% compatible (and we don't), then yes, two filters would be the better option:

I would then call the boolean = false option {{ var | defined_or(default_value) }}.
And the boolean = true option {{ expr | assigned_or(default_value) }}.

Fun fact minijinja does not implement |default either.

Kijewski · 2025-05-02T17:46:27Z

Or, what about both options?

{{ var | default(default_value, [false]) }} → {{ var | defined_or(default_value) }}
{{ expr | default(default_value, [boolean =] true) }} → {{ expr | assigned_or(default_value) }}

The filter |default will work as in jinja, same semantics, name argument naming; but you could also use filters with a (hopefully) better naming scheme.

GuillaumeGomez · 2025-05-02T19:59:02Z

Having two filters for the same thing sounds good to me. 👍

Just please improve the argument name. We can add an error for this specific case to tell that for better naming, askama is using X name instead for this named argument.

What do you think @joshka?

joshka · 2025-05-03T09:27:46Z

Supporting jinja syntax probably makes it easier to bring templates from elsewhere (and allows LLMs to fall into the pit of success by suggesting code which works even if it's sub optimal). So while I dislike the syntax, if there's a better syntax available like suggested above, then I've no complaints about also providing compatible syntax like this. I'd even go so far as to suggest that the parameter name is fine in that case (alternatively, if there's some future ability to alias it to give it an additional more descriptive name then that would work too)

Kijewski · 2025-06-01T21:37:50Z

I added |assigned_or and |defined_or as (better) alternatives to |defined.

I don't think that it would be a good idea to implement |defined with other arguments than in Jinja.

EITHER we keep it for Jinja compatibility exactly like in jinja
OR we drop it, because the other two filters are better
OR we re-implement the |default filter to compile_error! with a message to use the other filters.

joshka · 2025-06-01T22:18:26Z

The other names are good.

I like the idea of making it possible to do something useful if someone tries jinja syntax. That makes a lot of sense as it makes it easy to fall into a pit of success.

But I also like the idea of making this a compile error, but wonder if it might be possible to cause a deprecation warning to be emitted for default instead? Are there cases where a template might want to need default|undefined type logic in askama? I.e. is there some case where a template would call another template but intentionally not provide some field?

Kijewski · 2025-06-01T22:39:48Z

~~Blocked by #471.~~

Are there cases where a template might want to need default|undefined type logic in askama? I.e. is there some case where a template would call another template but intentionally not provide some field?

I could not think of a use case. In python, the boolean argument can be evaluated at runtime, but with our pre-compiled code this is not possible.

Right now, a proc-macro can only emit fatal errors, no warnings. There are open RFCs to add compile_warning!-like macros, though. But I guess we could always add a call to ↓ to fake a deprecation message.

#[deprecated(note = "use `|defined_or` or `|assigned_or` instead of `|default`")]
fn dont_use_default() {}

Most warnings that come from proc-macros get suppressed, though. I would need to test if deprecation message are one of them.

Also, `enum Pluralize<S, P>` is renamed into `enum Either<L, R>` and exported.

Kijewski · 2025-06-06T15:11:54Z

We can't assign a span to the warning (→ #420). Without the span, such a warning is kinda useless IMHO. I think the current implementation is fine as it is. :) We can always add a warning in a subsequent PR. @GuillaumeGomez, @joshka, what do you think? Good enough to be merged?

GuillaumeGomez · 2025-06-06T21:56:16Z

Output looks ok to me.

joshka

Some extra rview notes having gone through this in a bit more detail.

joshka · 2025-06-13T19:58:00Z

askama/src/filters/builtin.rs

+    fn as_filtered(&self) -> Result<Option<Self::Filtered<'_>>, Self::Error>;
+}
+
+const _: () = {


Got it. I know that often there's a tendency for larger module sizes in some Rust projects. This seems to be a symptom of that. I tend to address the same core problem (strongly related code that isn't in the outer scope) by using smaller narrower module (files) and liberal re-exports instead. This keeps the unit tests fairly close to the code it's testing, makes it easy to navigate and find code, etc. Obv. this is subjective, but maybe something to think about in future.

So a soft suggestion to extract this to filters/default.rs or something.

joshka · 2025-06-13T20:05:31Z

askama/src/filters/builtin.rs

+    label = "`{Self}` is not `|assigned_or` filterable",
+    message = "`{Self}` is not `|assigned_or` filterable"
+)]
+pub trait DefaultFilterable {


It could be worth adding a unit tests for the various implementations of this that captures each implementation. E.g.:

assert_eq!(0_u8.as_filtered(), Ok(None)); assert_eq!(1_u8.as_filtered(), Ok(Some(1));

joshka · 2025-06-13T20:09:49Z

book/src/filters.md

+
+```jinja
+{% let greeting = Some("Hello") %}
+{{ greeting.as_ref() | default("Hi", true) }}


should be

Suggested change

{{ greeting.as_ref() | default("Hi", true) }}

{{ greeting.as_ref() | assigned_or("Hi") }}

joshka · 2025-06-13T20:10:20Z

book/src/filters.md

+{{ variable_or_expression | assigned_or(fallback) }}
+{{ variable_or_expression | assigned_or(fallback) }}


duplicate line - needs to be fixed or state what's going on here.

joshka · 2025-06-13T20:13:22Z

book/src/filters.md

+This filter works like the Jinja filter of the same name.
+If the second argument is not an boolean `true`, then the filter behaves like [`|defined_or`][#defined_or].
+If it is supplied and `true`, then the filter behaves like [`|assigned_or`][#assigned_or].


This should maybe describe how this works without having to link to the jinja / the other filters.

joshka · 2025-06-13T20:14:48Z

book/src/filters.md

+If it is supplied and `true`, then the filter behaves like [`|assigned_or`][#assigned_or].
+
+**This filter exists for compatibility with Jinja.
+You should use [`|defined_or`][#defined_or] and [`|assigned_or`][#assigned_or] directly instead.**


Suggested change

You should use [`|defined_or`][#defined_or] and [`|assigned_or`][#assigned_or] directly instead.**

Askama provides [`|defined_or`][#defined_or] and [`|assigned_or`][#assigned_or] which both better express the intention and should generally usually be used instead of this filter.**

joshka · 2025-06-13T20:27:24Z

testing/tests/ui/default.stderr

+       "var | default }}"
+  --> tests/ui/default.rs:10:35
+   |
+10 | #[template(ext = "html", source = "{{ var | default }}")]


This actually suggests a bit of a jinja / rust impedance mismatch here. In Rust, we have a few different operations like map_or_default and unwrap_or_default. Intuitively, var | default when the type of var implements the Default trait seems like it would be obvious that it should return default. I know that a user could call .unwrap_or_default() or whatever there, but I'm curious whether it might be worth extending this to allow the obvious intuitive thing there. (I'm not fully certain of this, either way, WDYT?)

Kijewski · 2025-06-15T20:15:42Z

@joshka, thank you for your suggestions and proofreading! I will have a look tomorrow.

Kijewski force-pushed the pr-default branch 2 times, most recently from 4af0d79 to 90c531c Compare May 1, 2025 11:54

GuillaumeGomez reviewed May 1, 2025

View reviewed changes

askama_derive/src/generator/filter.rs Show resolved Hide resolved

GuillaumeGomez previously approved these changes May 1, 2025

View reviewed changes

joshka reviewed May 1, 2025

View reviewed changes

Kijewski dismissed GuillaumeGomez’s stale review via 7c19a9c May 1, 2025 22:20

Kijewski force-pushed the pr-default branch from 90c531c to 7c19a9c Compare May 1, 2025 22:20

Kijewski force-pushed the pr-default branch from 7c19a9c to 2516355 Compare May 1, 2025 22:33

joshka reviewed May 2, 2025

View reviewed changes

GuillaumeGomez reviewed May 2, 2025

View reviewed changes

askama_derive/src/generator/filter.rs Outdated Show resolved Hide resolved

GuillaumeGomez reviewed May 2, 2025

View reviewed changes

askama_derive/src/generator/filter.rs Outdated Show resolved Hide resolved

GuillaumeGomez reviewed May 2, 2025

View reviewed changes

Kijewski force-pushed the pr-default branch from 2516355 to 67620f4 Compare May 2, 2025 10:46

Kijewski force-pushed the pr-default branch from 67620f4 to 3c36fdc Compare May 2, 2025 11:16

Kijewski force-pushed the pr-default branch from 3c36fdc to a6a9d38 Compare June 1, 2025 21:30

Kijewski changed the title ~~Add filter |default~~ Add filters |default, |assigned_or and |defined_or Jun 1, 2025

Kijewski force-pushed the pr-default branch 4 times, most recently from eef03df to 826194d Compare June 1, 2025 21:35

Kijewski force-pushed the pr-default branch from 826194d to c307767 Compare June 2, 2025 19:50

Add filters |default, |assigned_or and |defined_or

6f5a0f6

Also, `enum Pluralize<S, P>` is renamed into `enum Either<L, R>` and exported.

Kijewski force-pushed the pr-default branch from c307767 to 6f5a0f6 Compare June 6, 2025 15:07

joshka reviewed Jun 13, 2025

View reviewed changes

		pub fn pluralize<C, S, P>(count: C, singular: S, plural: P) -> Result<Pluralize<S, P>, C::Error>
		pub fn pluralize<C, S, P>(count: C, singular: S, plural: P) -> Result<Either<S, P>, C::Error>

	{{ greeting.as_ref() \| default("Hi", true) }}
	{{ greeting.as_ref() \| assigned_or("Hi") }}

		{{ variable_or_expression \| assigned_or(fallback) }}
		{{ variable_or_expression \| assigned_or(fallback) }}

	You should use [`\|defined_or`][#defined_or] and [`\|assigned_or`][#assigned_or] directly instead.**
	Askama provides [`\|defined_or`][#defined_or] and [`\|assigned_or`][#assigned_or] which both better express the intention and should generally usually be used instead of this filter.**

Add filters |default, |assigned_or and |defined_or #425

Are you sure you want to change the base?

Add filters |default, |assigned_or and |defined_or #425

Uh oh!

Conversation

Kijewski commented May 1, 2025

Uh oh!

Kijewski commented May 1, 2025

Uh oh!

Uh oh!

GuillaumeGomez commented May 1, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshka commented May 1, 2025

Uh oh!

Kijewski commented May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshka Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kijewski commented May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kijewski commented May 2, 2025

Uh oh!

GuillaumeGomez commented May 2, 2025

Uh oh!

joshka commented May 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kijewski commented Jun 1, 2025

Uh oh!

joshka commented Jun 1, 2025

Uh oh!

Kijewski commented Jun 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kijewski commented Jun 6, 2025

Uh oh!

GuillaumeGomez commented Jun 6, 2025

Uh oh!

joshka left a comment

Choose a reason for hiding this comment

Uh oh!

joshka Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Add filters `|default`, `|assigned_or` and `|defined_or` #425

Add filters `|default`, `|assigned_or` and `|defined_or` #425

Kijewski commented May 1, 2025 •

edited

Loading

joshka Jun 13, 2025 •

edited

Loading

Kijewski commented May 2, 2025 •

edited

Loading

joshka commented May 3, 2025 •

edited

Loading

Kijewski commented Jun 1, 2025 •

edited

Loading

joshka Jun 13, 2025 •

edited

Loading