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.

Sign up for GitHub

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

Jump to bottom

Add display_some filter and friends #1007

Open
vallentin opened this issue Apr 16, 2024 · 14 comments · May be fixed by #1014
Open

Add display_some filter and friends #1007

vallentin opened this issue Apr 16, 2024 · 14 comments · May be fixed by #1014

Comments

@vallentin
Copy link
Collaborator

vallentin commented Apr 16, 2024 •
edited
Loading

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
Copy link
Owner

djc commented Apr 17, 2024

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
Copy link
Contributor

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

@Kijewski
Copy link
Contributor

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

I think it is.

@djc
Copy link
Owner

djc commented Apr 17, 2024

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

@GuillaumeGomez
Copy link
Contributor

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

@Kijewski
Copy link
Contributor

{% if let Some(x) = y %} is already implemented: #503. Or do I misunderstand what you mean?

@GuillaumeGomez
Copy link
Contributor

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

@djc
Copy link
Owner

djc commented Apr 17, 2024

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

@vallentin
Copy link
Collaborator Author

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
Copy link
Contributor

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
Copy link
Collaborator Author

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
Copy link
Owner

djc commented Apr 17, 2024

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
Copy link
Contributor

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
Copy link
Collaborator Author

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

@vallentin vallentin linked a pull request Apr 21, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Implemented display_some and display_some_or vallentin/askama
4 participants
@Kijewski @djc @GuillaumeGomez @vallentin