djc / askama

Type-safe, compiled Jinja-like templates for Rust
Apache License 2.0
3.35k stars 215 forks source link

Add display_some filter and friends #1007

Open vallentin opened 4 months ago

vallentin commented 4 months ago

The question of how to render a Option<T> comes up every now and then. So I think it's time to provide a simpler built-in solution.

Today, I answered a question on Stack Overflow, including 5 solutions. I think we should include a built-in display_some filter.

In #717 @djc suggested naming it or_empty.

_Personally, I think the name display_some is more clear, since val|or_empty, might be confusing for e.g. val: Option<i32> or any other Option<T> than Option<String>._


I've had a handful of display_* filters locally since forever. So I propose that we include (some of) the following:

pub fn display_some<T>(value: &Option<T>) -> askama::Result<String>
where
    T: std::fmt::Display;

pub fn display_some_or<T, U>(value: &Option<T>, otherwise: U) -> askama::Result<String>
where
    T: std::fmt::Display,
    U: std::fmt::Display;

Personally, I think display_some_or's otherwise should remain as U, instead of &Option<U>. Since the main need I've had over the years, have been in the form of e.g.:

{{ self.id|display_some_or("[no id]") }}

If I needed a.or(b).or(c).unwrap_or("default"), then I would instead do:

{% if self.a.is_some() %}
    {{ self.a|display_some }}
{% else if self.b.is_some() %}
    {{ self.b|display_some }}
{% else if self.c.is_some() %}
    {{ self.c|display_some }}
{% else %}
    default
{% endif %}

Even though this logic should probably be handled outside of the template in the first place.


Additionally, my local implementation doesn't actually return String, they return Display*, e.g. DisplaySome, where DisplaySome impl fmt::Display to avoid the String allocation.

So actually, what I want to add is:

pub fn display_some<T>(value: &Option<T>) -> askama::Result<DisplaySome<'_, T>>
where
    T: std::fmt::Display,
{
    Ok(DisplaySome(value))
}

...

Additionally, I also have display_* functions for Result, which might also be useful to include:

pub fn display_ok<T, E>(value: &Result<T, E>) -> askama::Result<String>
where
    T: std::fmt::Display;

pub fn display_err<T, E>(value: &Result<T, E>) -> askama::Result<String>
where
    E: std::fmt::Display;

pub fn display_ok_err<T, E>(value: &Result<T, E>) -> askama::Result<String>
where
    T: std::fmt::Display,
    E: std::fmt::Display;

_The display_ok_err() is mainly useful for debugging._

The display_ok and display_err filters, are mainly useful when you want to render Ok vs Err in separate places, e.g.

{% if self.res.is_ok() %}
    {{ self.res|display_ok }}
{% endif %}

...

{% if self.res.is_err() %}
    Error: {{ self.res|display_err }}
{% endif %}

Compared to:

{% match self.res %}
    {% when Ok with (val) %}
        {{ val }}
    {% else %}
{% endmatch %}

...

{% match self.res %}
    {% when Err with (err) %}
        Error: {{ err }}
    {% else %}
{% endmatch %}

I have the implementation locally, so I just need some opinions on which filters to include as built-in, and any other comments, before making a PR for them.

djc commented 4 months ago

I like display_some and display_some_or, and those are pretty good names. @GuillaumeGomez @Kijewski any thoughts?

I'm not as convinced about the Result filters for now.

GuillaumeGomez commented 4 months ago

Wouldn't it be better to support if let Some(value) = val instead?

Kijewski commented 4 months ago

Wouldn't it be better to support if let Some(value) = val instead?

I think it is.

djc commented 4 months ago

That's fair, too. How much of the current match parsing could we reuse for that?

GuillaumeGomez commented 4 months ago

Didn't check yet but I suppose it shouldn't be too difficult to add.

Kijewski commented 4 months ago

{% if let Some(x) = y %} is already implemented: https://github.com/djc/askama/pull/503. Or do I misunderstand what you mean?

GuillaumeGomez commented 4 months ago

TIL. Well then, is there any point in having a filter in this case?

djc commented 4 months ago

@vallentin any thoughts? Were you aware if let is supported now?

vallentin commented 4 months ago

I even reviewed #503, but somehow completely forgot about it.

Personally, I would still say that this:

{{ id|display_some }}

{{ id|display_some_or("default") }}

Is less verbose than this:

{% if let Some(id) = id -%}
    {{ id }}
{%- endif %}

{% if let Some(id) = id -%}
    {{ id }}
{%- else -%}
    default
{%- endif %}

Personally, I prefer removing as much logic from templates as possible.

Additionally, using if let requires you to think about the whitespace control.

However, given that if let is supported, then I would understand, if these don't cut it as a built-in filters, even though I would personally advocate for having them.

GuillaumeGomez commented 4 months ago

I'd argue that having more filters to have a shortcut on what's already possible is not a great added value. In addition, in here the difference is really small, so I personally don't think it's worth it.

vallentin commented 4 months ago

I do understand, as I completely acknowledge my bias in this case, since I've grown accustomed to using them and having short and concise logic-less templates

djc commented 4 months ago

Actually I kinda like display_some and display_some_or and I'm sensitive to the conciseness argument for something that's arguably pretty common.

Kijewski commented 4 months ago

I am in favor of the new filters, too! {{ data|display_some_or("?") }} is much more readable than

{% if let Some(data) = data -%}
    {{ data }}
{%- else -%}
    ?
{%- endif %}
vallentin commented 4 months ago

Alright, I'll make a PR for it. I have all the code ready anyways, just need to update the book