zendesk / curly

The Curly template language allows separating your logic from the structure of your HTML templates.
593 stars 19 forks source link

Accept methods with more than one parameter #61

Closed thelinuxlich closed 10 years ago

thelinuxlich commented 10 years ago

Why not?

dasch commented 10 years ago

How would you imagine the syntax to be? Currently {{foo.bar.baz}} invokes foo with the argument "bar.baz" - we're using that convention at Zendesk. How would you separate the other arguments?

thelinuxlich commented 10 years ago

{{ cycle:red,blue }}

dasch commented 10 years ago

So {{cycle:red,blue}} would invoke cycle(["red", "blue"])?

dasch commented 10 years ago

Then I'd prefer something more flexible: how about {{cycle.values: red, blue foo: bar}} -> cycle("values" => ["red", "blue"], "foo" => "bar")

dasch commented 10 years ago

@thelinuxlich how would {{foo:bar}} be evaluated? foo(["bar"])?

teliosdev commented 10 years ago

@dash {{cycle.values: red, blue foo: bar}} is confusing IMO, because it looks like { "cycle.values" => ["red", "blue"], "foo" => "bar" }. You could change the behaviour of {{foo.bar.baz}} based on the number of arguments foo() takes (i.e., if it takes one argument, pass it "bar.baz"; if it takes more than one, pass it "bar" and "baz" as two separate arguments).

thelinuxlich commented 10 years ago

Take a look at the tests, {{ cycle:red,blue }} would call a cycle method with red and blue parameters. IMO {{cycle.values: red, blue foo: bar}} is confusing. I was aiming at something simple like Liquid filters, we assume every parameter will be a string and the user take care of that inside the presenter method.

dasch commented 10 years ago

@thelinuxlich I'd just like to not have two different ways of passing parameters, {{foo.bar}} and {{foo:bar}}.

@medcat I agree that it's confusing. I'm not sold on introspecting the method and splitting based on that...

thelinuxlich commented 10 years ago

@dasch I think that passing method parameters with dot should be deprecated. It's unintuitive, "foo.bar.baz" doesn't seem like invoking a foo method with bar and baz parameters.

teliosdev commented 10 years ago

@thelinuxlich that's too much of a jump. Using that logic, {{ cycle:red,blue }} doesn't really make sense either.

thelinuxlich commented 10 years ago

@medcat your proposal doesn't look like a method invocation with parameters either, I would like an explanation why {{ method:param1,param2 }} doesn't make sense

dasch commented 10 years ago

It's not supposed to look like method invokation – that's the entire point of Curly. It's supposed to be a parameterized variable reference, or a "component" that can be placed in the source.

Curly is a declarative templating language, so any construct should abstract away the fact that methods are used to implement this stuff.

@thelinuxlich what's your concrete use case for this?

thelinuxlich commented 10 years ago

@dasch Just take a look at Liquid filters, there is plenty of use cases for this. I don't see the point of blocking method invocation.

thelinuxlich commented 10 years ago

What if the user editing the views, which doesn't have access to the Presenters, would like to format a price/name/url/whatever_text_you_got in a custom way?

yurifrl commented 10 years ago

i like this approach, although i prefer named params, something like the php hash +1 to the request

lucaspalencia commented 10 years ago

+1 to the request!

dasch commented 10 years ago

@thelinuxlich Curly is intentionally not like Liquid. Its components are much higher-level, and procedural code is avoided. We actually extracted this from our own product, where our customers are able to customize their sites by editing Curly templates. Because we wish to support backwards compatiblity, we intentionally made components coarse-grained.

Curly will never become like Liquid – Liquid is like Liquid :-)

Therefore, custom formatting of raw data is a bad example. Components should always be strings of HTML that are injected where they're placed in the source. The only reason an argument is currently supported is in order to provide a richer vocabulary of components, e.g. we use it for injecting localized snippets of html: {{i18n.help.welcome_text}}.

dasch commented 10 years ago

I'd like to see some Curly-esque use cases for this before proceeding.

thelinuxlich commented 10 years ago

@dasch I totally understand it, and Curly not being like Liquid was what brought me here. I can see many use cases for frontend styling:

<tr class="{{ cycle:red,blue }}">
dasch commented 10 years ago

@thelinuxlich I'd like to support better parameterization of components/references, but I'm hesitant until I'm 100% sure it's something we can support going forward. I'm not necessarily opposed to adding a second way of referencing stuff (e.g. {{foo:bar}}), but it needs to be flexible enough to handle different use cases. What concrete use case isn't covered by the current capabilities?

thelinuxlich commented 10 years ago

I'm porting all Spree views to Curly, I'm spotting many times where a method invocation with parameters should be doable, because non-expert users will be modifying it after my port.

So if I have something like {{ display_price }} and the user wants to customize the format? Write a method for every format possible?

Or if I have something like {{ products }} for displaying them but the user wants to customize how many products are shown, and maybe another characteristic like how many per row?

dasch commented 10 years ago

We've debated these things internally as well. So far, we've always opted for a more explicit, if less DRY, approach where we add more methods to the presenter:

  1. Yeah, I'd probably add a method per format. How much customizability do you want there?
  2. So far we've resisted such requests in order to more easily provide updates – maintaining backwards compatibility with a heavily customized component is difficult. You could, however, have your own DSL for starters:

    {{products.max: 30 per_row: 3}}
    def products(format = nil)
     # parse the format string, it's all in there.
    end

    That would be very flexible, but of course much more difficult to implement and maintain.

There's actually a backwards compatible syntax that I've been contemplating:

{{products max: 30 per_row: 3}}

Since there's a space in there, it's not a valid reference at the moment. It could even be combined with a normal positional argument: this would break backwards compatibility, probably either positional or named args for now...

brunopazz commented 10 years ago

+1 to the request! Awesome!

thelinuxlich commented 10 years ago

seems viable

antoniosb commented 10 years ago

+1

leandropincini commented 10 years ago

+1

thelinuxlich commented 10 years ago

Since we are into a pattern for presenter methods with arguments, I would like to enforce something like keyword arguments, would be much more clean:

{{ products max:30 per_row:5 }}
def products(max:10, per_row:3)
    # no need to parse anything
end
dasch commented 10 years ago

I'll be tracking this issue in #65.