search

tbranyen / combyne

A template engine that works the way you expect.
MIT License
144 stars 19 forks source link

Ensure that conditional arguments are parsed valid #81

Closed tbranyen closed 9 years ago

tbranyen commented 9 years ago

While working on a sample benchmarking app, I noticed that the following was invalid:

{%each databases as database name%}{{database}}: {{name}}{%endeach%}

Using the the object:

{
  "database1": "online"
}

I was unable to get the template to render correctly.

After inspecting the template.tree object, I was able to discover that the tokenizer had correctly found two cases of ASSIGNMENT the as in database.

I updated the AST generation code to account for actual assignment and properly adjust depending on what's passed.

kadamwhite commented 9 years ago

Unclear on the language of "two cases of ASSIGNMENT the as in database" -- feel like there's an article missing. Is the issue that "as" is a substring of DatabASe?

tbranyen commented 9 years ago

Yup, the tokenizer can't differentiate and probably shouldn't. This logic seems to make most sense in the tree builder.

kadamwhite commented 9 years ago

I'm not sure I agree (or possibly don't follow the rationale). as (spaces) vs [^\s]as[^\s] seem like very different types of token, to me -- but I also don't know much about how the tokenizer works, so I may be missing a deeper reason that this needs to be handled by a conditional instead of not being parsed as a non-identifier token in the first place

tbranyen commented 9 years ago

@kadamwhite here's the grammar that the tokenizer uses to parse 'as' as ASSIGN https://github.com/tbranyen/combyne/blob/master/lib/grammar.js#L41

Since it parses for all of them, even within other words, it's up to the tree builder to understand how to string them together. The tokenizer's job shouldn't be to try and understand context of where it's used.