Skip to content
This repository was archived by the owner on Mar 11, 2025. It is now read-only.

Add format filter that swaps the first two arguments. - #345

Merged
djc merged 1 commit into
askama-rs:masterfrom
couchand:202007/fmt-swap
Jul 14, 2020
Merged

Add format filter that swaps the first two arguments.#345
djc merged 1 commit into
askama-rs:masterfrom
couchand:202007/fmt-swap

Conversation

@couchand

@couchand couchand commented Jul 8, 2020

Copy link
Copy Markdown
Contributor

Swapping the first two arguments allows a more natural filter usage: {{ val | fmt("{:?}") }}.

Happy to bikeshed the name.

@djc

djc commented Jul 8, 2020

Copy link
Copy Markdown
Collaborator

I don't see this adding much value -- it seems more likely to be confusing. Sorry!

@djc djc closed this Jul 8, 2020
@couchand

couchand commented Jul 8, 2020

Copy link
Copy Markdown
Contributor Author

If you don't want to add this, that's fine, but I hope you can at least take a moment to consider how this API is significantly more ergonomic.

For one thing, it better matches the mental model of other filters: data flows in from the left, configuration is passed as a param (see, for instance, join). It is also superior for composition with other filters, such as in {{ num | to_f64 | fmt("{:.2}") }}.

@djc

djc commented Jul 8, 2020

Copy link
Copy Markdown
Collaborator

Should have led with that! I hadn't realized the composition advantage. How do you feel about restricting it to a single argument (so no more arguments after the format string)?

@djc djc reopened this Jul 8, 2020
@couchand

couchand commented Jul 8, 2020

Copy link
Copy Markdown
Contributor Author

Limiting it to one argument makes sense, it would get awfully confusing with more than one.

For that matter, I'm not sure there's much point to putting any static text in the format string in the context of a templating language. Would it be crazy to automatically wrap the format in curly braces?

{{ num | fmt(":.2") }}

On the other hand, perhaps there would be a reason to include static text, to support composition with downstream filters:

{{ price | fmt("${:.2}") | center }}

Though that suggests a use-case for additional parameters (most useful if named) :

{{ price | fmt("{symbol}{:.2}", symbol=currency) | center }}

Although at that point you're probably better off writing a custom filter.

@djc

djc commented Jul 9, 2020

Copy link
Copy Markdown
Collaborator

One of the core design choices in Askama is to make the templating language Rust-like. Eliding the braces seems antithetical to that.

Let's start with the simple two-argument version in this PR and we can discuss extending it once we run into a compelling use case.

@couchand

couchand commented Jul 9, 2020

Copy link
Copy Markdown
Contributor Author

Updated. What do you think about the name? I'd prefer to be explicit about the difference. Perhaps format_with or format_as would clarify it?

As an aside, have you considered using trybuild to test compile fails?

@djc djc left a comment

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.

Some minor nits. Please also squash all the commits together into a single one.

I think the name is probably okay. Compound names with _ seem a bit ugly -- I'm open to any non-compound alternatives you come up with? Might be nice to use the documentation to point from one to the other and vice versa, though. And ideally, also update the book sources.

Comment thread testing/tests/filters.rs Outdated
Comment thread askama_shared/src/filters/mod.rs Outdated
This allows a more natural filter usage: `{{ val | fmt("{:?}") }}`
as well as enabling convenient filter composition:
`{{ price | to_f64 | fmt("${:.2}") | center }}`
@couchand

Copy link
Copy Markdown
Contributor Author

Updated per all feedback and squashed the commits. I'm happy with the name; given docs examples and links both ways it seems pretty discoverable.

Let me know if you've got any other thoughts. Thanks!

@djc
djc merged commit 42c4cb1 into askama-rs:master Jul 14, 2020
@djc

djc commented Jul 14, 2020

Copy link
Copy Markdown
Collaborator

Thanks for your contribution, much appreciated (especially for sticking with it despite the initial negative feedback)!

@couchand

Copy link
Copy Markdown
Contributor Author

Totally understand, I should have provided more context from the start. Thanks!

@Kijewski Kijewski mentioned this pull request Jan 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants