-
-
Notifications
You must be signed in to change notification settings - Fork 221
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
Add display_some filter and friends #1007
Comments
I like I'm not as convinced about the |
Wouldn't it be better to support |
I think it is. |
That's fair, too. How much of the current |
Didn't check yet but I suppose it shouldn't be too difficult to add. |
|
TIL. Well then, is there any point in having a filter in this case? |
@vallentin any thoughts? Were you aware |
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 However, given that |
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. |
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 |
Actually I kinda like |
I am in favor of the new filters, too! {% if let Some(data) = data -%}
{{ data }}
{%- else -%}
?
{%- endif %} |
Alright, I'll make a PR for it. I have all the code ready anyways, just need to update the book |
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, sinceval|or_empty
, might be confusing for e.g.val: Option<i32>
or any otherOption<T>
thanOption<String>
.I've had a handful of
display_*
filters locally since forever. So I propose that we include (some of) the following:Personally, I think
display_some_or
'sotherwise
should remain asU
, 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: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 returnDisplay*
, e.g.DisplaySome
, whereDisplaySome
implfmt::Display
to avoid theString
allocation.So actually, what I want to add is:
Additionally, I also have
display_*
functions forResult
, which might also be useful to include:The
display_ok_err()
is mainly useful for debugging.The
display_ok
anddisplay_err
filters, are mainly useful when you want to renderOk
vsErr
in separate places, e.g.Compared to:
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.
The text was updated successfully, but these errors were encountered: