bkiers / Liqp

An ANTLR based 'Liquid Template' parser and rendering engine.
MIT License
165 stars 94 forks source link

Allow keywords like "offset" in for/assign/capture/tablerow #252

Closed kohlschuetter closed 1 year ago

kohlschuetter commented 1 year ago

Currently we require that the variable names in these blocks/tags are of type "Id", which prevents use to use variable names like "offset". These are commonly used in Jekyll and perfectly fine otherwise.

Change the parser to allow the existing type "id" (lower case) instead of "Id" in these scenarios.

kohlschuetter commented 1 year ago

FYI @bkiers

kohlschuetter commented 1 year ago

@bkiers @msangel Any objections against this change? Thanks in advance for reviewing!

msangel commented 1 year ago

As for me: just add more test cases where the reserved/extended ids used. For example like this: "{% for raw in (1..3) %}{{ raw }}{% endfor %}"

kohlschuetter commented 1 year ago

I can do that, but would we be actually hitting different codepaths with them (versus just spending more time testing the same change)?

msangel commented 1 year ago

My concern is that some reserved words will be no longer reserved. The behavior must be like in ruby’s version, including reproduction their errors.

kohlschuetter commented 1 year ago

Which reserved words are you thinking of? Aren't non-reserved words already covered by other tests? (This PR fixes one of the few remaining bugs blocking rendering my blog's Jekyll theme, with Java code powered by Liqp)

msangel commented 1 year ago

Had to say that this implementation goes in line with ruby behavior image

msangel commented 1 year ago

@kohlschuetter thank you!