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.

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

Enhance filters to accept variables, using syntax similar to Liquid or Twig #264

Closed
wants to merge 6 commits into from

Conversation

sethkinast
Copy link
Contributor

Hi!

I've added support for Dust's filters to accept arguments. This allows for syntax such as:

{foo|replace:"bar","baz"}

Or even:

{some_text|link_to:"{url}"}

The syntax is the same as that supported by Twig, the popular PHP templating language, and Liquid, which is used by lots of Ruby projects including Shopify. Filter variables are extremely useful when you need to do some simple string manipulation that is too simple to need a helper! Some examples of filters that use variables in Liquid: https://github.com/Shopify/liquid/wiki/Liquid-for-Designers

The change is completely backwards-compatible. All tests still pass. I also added six new tests around the new functionality-- please check them out to get an idea of how filter variables work.

To have something to test with, I did add two filters to dust.filters-- "replace" and "link_to". I understand those probably don't permanently belong in the core but I know there is some movement around filters anyways so hopefully they can find a temporary home in core and migrate on when we have a place for them.

As an unrelated change, I noticed that the browser test suite had some incorrect logic that was removing some of the test cases erroneously, so I've fixed that as well.

As this is a new feature and a backwards-compatible change, I also incremented the minor version, making this Dust 1.3.0.

Thank you for reviewing this change!

@vybs
Copy link
Contributor

vybs commented Apr 25, 2013

Thanks Seth. Would not the helpers in dust achieve the same? Do filters need to be extended for the same thing helpers already provide?

@sethkinast
Copy link
Contributor Author

Hi,

So I would suggest that filters and helpers have somewhat different roles. Helpers are for more complex tasks that might involve multiple items from the context stack, iteration, etc.; filters are for quick string manipulation.

I personally find it nice to be able to do something like

{some_number|as_decimal:"3"}

Rather than have to do:

{@Decimal num="{some_number}" precision="3" /}

Or:

{my_name|link_to:"{my_profile}"}

Instead of:

{?my_profile}<a href="{my_profile}">{/my_profile}{my_name}{?my_profile}</a>{/my_profile}

But I do agree that this doesn't enable anything that you couldn't do already! Just some syntax sugar that will be familiar to those coming from some other ctemplate-like languages :) so it is not essential to add if you don't think it would be a good fit. It's something I've found very useful, though.

I also forgot to mention one facet of the pull request-- it also allows filters to negate the automatic HTML-escaping by setting context.auto to null inside the filter. (The link_to filter does this)

@vybs
Copy link
Contributor

vybs commented Apr 25, 2013

Hmm... there are some use cases this syntax may be useful and certainly it is very neat.!

We should discuss with others on the thread and will need some time to review.

@@ -76,10 +76,13 @@ partial "partial"
{ var key = (s ===">")? "partial" : s; return [key, n, c, p] }

/*-------------------------------------------------------------------------------------------------------------------------------------
filters is defined as matching a pipe character followed by anything that matches the key
filters is defined as matching a pipe character, optionally surrounded by whitespace, followed by anything that matches a key,
optionally followed by a colon and an inline param
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would be nice to support inline & identifier similar to how params in the section works.

{myData: 'hello'}
e.g. {foo|replace:"placeholder", myData}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's smart. I'll have to grok some more PEG.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the pull request. This makes the syntax even nicer when I can pass numerics and inlines directly! Thanks for the catch.

@jimmyhchan
Copy link
Contributor

Thanks for the pull request. Lots of good work.

I like your definition of filter vs helper. We should definitely open this up more. Internally we have thought of filters as core functions that are universal and helpers are extension stuff that are specific to people's projects. Replace and link_to are neat new feature but it's not as core to Dust as basic html/js encoding. I'm not saying it's not useful, just wondering what do we do for folks who don't want to use this?

I think at the heart of this. Do we want Dust core to support filter parameters?
e.g. {foo | filter:"value1", "value2"} or `{foo | filter key1="value1" key2=value2}

If this passes, then do we want these implementations (replace/link_to) in core or via some extension?

@sethkinast
Copy link
Contributor Author

I think regardless of whether you take this or not, replace and link_to don't belong in core. As I mentioned, they're mainly there at the moment to have something to write test suites around, and I expect(ed) them to move out of core.

I have future PRs around the concept of "registering" filters and allowing you to easily modularize them.

…in addition to literals and variables already supported)
@rragan
Copy link
Contributor

rragan commented Apr 26, 2013

See also #204 and #205 re filters. One advantage not mentioned is that filters can be used where a {key} reference can be used which is not true of helpers. One could thus have a size filter that could generate a length that could actually be passed as a param, unlike the current size helper. I suspect this could lead to some interesting capabilities but one would want user definable ones to keep the dust from burgeoning.

@jimmyhchan
Copy link
Contributor

@sethkinast what do you about the syntax {foo | filter key1="value1" key2=value2} instead. The grammar can be a little simpler as f:("|" n:key p:params {return [n,p]})*

Regarding using new Chunk to dereference the data, I don't have a better idea at the moment, but I'm leaning towards trying to work it out in the compiler or grammar... but it might not be possible. There's also the tap method which I'm not too familiar with but that's how we were doing things in the helpers side. We essentially need to support nested references {foo|{foo}}

as far as next steps, before we spend too much time polishing this PR

Let's vote:
Do we want Dust core to support filter parameters?
e.g. {foo | filter:"value1", "value2"} or {foo | filter key1="value1" key2=value2}

+1 for me let's support it.
@rragan, @vybs @jairodemorais @iamleppert @smfoote wdyt

@smfoote
Copy link
Contributor

smfoote commented May 1, 2013

+1 - let's add good documentation about what should be a helper and what
should be a custom filter.

On Wed, May 1, 2013 at 9:30 AM, jimmyhchan notifications@github.com wrote:

@sethkinast https://github.com/sethkinast what do you about the syntax {foo
| filter key1="value1" key2=value2} instead. The grammar can be a little
simpler as f:("|" n:key p:params {return [n,p]})*

Regarding using new Chunk to dereference the data, I don't have a better
idea at the moment, but I'm leaning towards trying to work it out in the
compiler or grammar... but it might not be possible. There's also the tapmethod which I'm not too familiar with but that's how we were doing things
in the helpers side. We essentially need to support nested references
{foo|{foo}}

as far as next steps, before we spend too much time polishing this PR

Let's vote:
Do we want Dust core to support filter parameters?
e.g. {foo | filter:"value1", "value2"} or {foo | filter key1="value1"
key2=value2}

+1 for me let's support it.
@rragan https://github.com/rragan, @vybs https://github.com/vybs
@jairodemorais https://github.com/jairodemorais @iamlepperthttps://github.com/iamleppert
@smfoote https://github.com/smfoote wdyt


Reply to this email directly or view it on GitHubhttps://github.com//pull/264#issuecomment-17290457
.

@rragan
Copy link
Contributor

rragan commented May 1, 2013

+1 for parameters. They seem to add significant value. In the interests of keeping filters simple, I would be in favor of simple positional parameters. It seems like key key=value starts to tread on the domain of helpers in terms of adding complexity. Filters should be modest in what they undertake.

They remind me of the distinction in JSP between custom tags and EL functions. The latter have positional parameters and can appear in expressions while the tags cannot. Or the not quite so analogous distinction between span and div (inline vs block).

@sethkinast
Copy link
Contributor Author

@jimmyhchan I think I have to echo @rragan in that keeping filter params positional would help to both reduce the complexity that can be jammed into a filter and also serve to better-distinguish them from helpers.

@vybs
Copy link
Contributor

vybs commented May 3, 2013

I still think key value pairs is good for long term, since positional params may expect a ordering someone needs to remember. Contrary to keeping them separate, having one way to do params is even better to me.

@sethkinast
Copy link
Contributor Author

I guess I have to disagree in that filters should be kept very simple. You should never have a filter that takes five or six optional parameters, for example.

The purity of being able to use {foo|filter:"param"} as the most common (by a long shot) case outweighs the flexibility of having to do {foo|filter:param="value"} every time.

updates to escapeJS
@vybs
Copy link
Contributor

vybs commented Jun 3, 2013

isn't the {text|truncate size="60"} expressive? what are we losing by keeping is {text|truncate:60}

@rragan
Copy link
Contributor

rragan commented Jun 3, 2013

If you are suggesting having both keyword and positional, I'm not sure at first glance how to convey a mixture of the two or to write a filter that could accept either. We should go with one or the other. I agree size="60" is more expressive and easier to understand the intent at a glance. I worry though that one of the gains with filters is the ability to use them in params might vanish if we add param-like syntax to them. I suspect you make life hard on the compiler.

{@if cond="{@SiZe key='abc'/} < 10} }.... is not something we can parse today and the quotes in quotes gets hard to read. Contrast with {@cond ="{abc|size}" < 10} (assuming we had size as a filter). This is already parseable and, to me, easier to read. I'm in favor of extending filters but not into the space of helpers.

@vybs
Copy link
Contributor

vybs commented Jun 4, 2013

named values is extensible than ordered values, implementing something as filter vs helper can get hairy at times. it can be done in both aways, so flexible and consistent is better than having to deal with 2 different ways

@vybs vybs closed this Jun 4, 2013
@vybs vybs reopened this Jun 4, 2013
@vybs
Copy link
Contributor

vybs commented Jun 4, 2013

sorry, accidentally closed it. no intention. Really love this feature and want to see it merged soon.!

@sethkinast
Copy link
Contributor Author

I think that if you have more than 1 or 2 positional parameters, that is a signal that your code should not be a filter.

I really suggest that the ability to have very succint filters, along with @rragan's comments that it is very hard to nest params into a param, make the roles of filters and helpers different.

I would make the case for strong documentation about use cases for filters vs helpers in lieu of making them effectively the same thing. (If they are the same there is little reason to ever use a filter.) I would be happy to assist in the writing of said documentation.

@vybs
Copy link
Contributor

vybs commented Jun 4, 2013

a param within a param is nto even valid grammar today, so does not apply in filters as well.

So why is having a named param not succint? more characters to type is it?

@sethkinast
Copy link
Contributor Author

Yeah... hm, let me try to explain my thoughts.

Pros of having positional parameters:

  • Succint to type ({foo|truncate:3} vs {foo|truncate: length=3}) and you don't have to remember the name of the param. Also it is easy to see which params belong to which filter.
  • Devs used to Liquid, Twig, Django, etc. already will be familiar with this syntax.
  • Provides clear indication to filter creators when their use case is probably wrong for a filter (do you have 5 params, 4 of which are optional? Probably a bad filter because its scope is too broad.)

Pros of having named parameters:

  • Clearer what the parameter does (however I would suggest that most of the time the intent should be obvious from the filter name)
  • Can have optional parameters / out-of-order parameters (unclear if this is a benefit though)
  • One syntax for both helpers and filters (again unsure if benefit because it muddies the distinction)

Filters, it seems to me, should be as logicless as possible. Named parameters seem to introduce the ability to have some simple boolean logic in the form of "parameter is present / absent", so one filter could have many different outputs, which seems to be more of a helper's role. I dunno!

@rragan
Copy link
Contributor

rragan commented Jun 5, 2013

Re the comment about a param within a param not being valid: maybe I was unclear.

This is valid today: {@SiZe key="{str|h}"/} and if we had a size helper in filter form (which was discussed) we could do things like {@if cond="{str|size} > 20} .... {/if} which cannot be written with the size helper because helpers cannot use other helpers in param values.

The current compiler treats either form as simply "output the mess as a string" so there is compiler work with either approach. Another pro for named parameters is that presumably the evaluation rules for p=1, p=name, p="{name}" would be intuitively understood to be like helpers. I would hope that the positional form would end up with equivalent generated code {str|filter 1, name, "{name}"} but the user might not make the same connection to helper rules.

That said, I still prefer positional parameters because it forces filters to be simpler and closer to logic-less.

@rragan
Copy link
Contributor

rragan commented Aug 4, 2013

I'd like to resurrect this issue again. There seemed to be a decent amount of sentiment for it as a language extension mechanism. The main point of debate where we left this was whether params should be positional or keyword.

I noticed that handlebars (like JSP) makes the distinction between extensions that can be used like an expression and those that are block oriented. http://blog.teamtreehouse.com/handlebars-js-part-2-partials-and-helpers

We could consider a more Handlebars like syntax for adding parameters, viz, {name params} instead of {string | name params} which is sort of a method like invocation. That would leave filters much as they are in the language and introduce an alternate extension vector (dust.functions). Like handlebars and JavaScript, the parameters would be positional. We probably would want {name params | filters} so function output could be filtered.

@sethkinast sethkinast closed this Oct 30, 2013
@kurdin
Copy link

kurdin commented Mar 24, 2016

Sorry I did not read whole thread but can we use it already ?

({foo|truncate:3}

This is super useful and shorter then using helpers, and you can chain filters.

@kurdin
Copy link

kurdin commented Mar 24, 2016

Looks like, currently you can not pass any params to filters. Please add this feature, it would be very useful.

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants