Skip to content

cleanup whitespace after tags (jinja style)#194

Closed
avalanche123 wants to merge 2 commits intoShopify:masterfrom
avalanche123:cleanup-whitespace
Closed

cleanup whitespace after tags (jinja style)#194
avalanche123 wants to merge 2 commits intoShopify:masterfrom
avalanche123:cleanup-whitespace

Conversation

@avalanche123
Copy link
Copy Markdown

This removes extra whitespace around tags, just like Jinja2 does

template:

items:
{% for item in items %}
  - {{ item }}
{% endfor %}

before:

items:

  - 1

  - 2

  - 3

after:

items:
  - 1
  - 2
  - 3

@nickpearson
Copy link
Copy Markdown
Contributor

Can this be made configurable, or done by changing the syntax of the tag itself? For example, by adding ERB-style hyphens, such as in #171:

{%- for item in items -%}

In my opinion, Liquid shouldn't mess with the whitespace unless told to do so, because it means you don't have complete control over the output. This would present a problem when generating content where whitespace matters, such as in plain-text emails.

In other words, having control over the output is more important than having pretty templates. The output you're looking for could be accomplished simply by moving the {% for %} and {% endfor %} tags:

items:{% for item in items %}
  - {{ item }}{% endfor %}

@phoet
Copy link
Copy Markdown
Contributor

phoet commented Jun 4, 2013

i think that the points of @nickpearson are quite valid. it also breaks a lot of tests, so its not a backwards compatible change.

@avalanche123 is this a mayor feature for you?

@oliverrauscher
Copy link
Copy Markdown

I vote for a configuration option.

Meanwhile in my use cases, I found a work around by just "defining" to have a '' (backslash) at the end of a row as a "no line break" character as common in most languages. So I just run the render and do a gsub("\n","") on the result:

liquid = Liquid::Template.parse(content)
liquid.render( { ... } ).gsub("\\\n","")

So you can use the \ character in your template as follows:

This should \
all go in \
one line

results in ...

This should all go in one line

@nickpearson
Copy link
Copy Markdown
Contributor

I'm unable to try this out right now, but for the sake of discussion, would adding this behavior be as simple as changing some of the regex definitions to chop off the leading newline+whitespace and the trailing newline if there is a hyphen in the right place (mimicking ERB's <%- and -%> syntax)?

For example, starting on line 27 of liquid.rb, here's something that might work to enable tags to start with {%- and end with -%} and for output segments to start with {{- and end with -}}

  TagStart                    = /(?:\r?\n?[ \t]*\{\%-|\{\%)/   # was: /\{\%/
  TagEnd                      = /(?:-\%\}[ \t]*\r?\n?|\%\})/   # was: /\%\}/
  # ...
  VariableStart               = /(?:\r?\n?[ \t]*\{\{-|\{\{)/   # was: /\{\{/
  VariableEnd                 = /(?:-\}\}[ \t]*\r?\n?|\}\})/   # was: /\}\}/

Some notes on the implementation:

  • Each of these still contain the original regex, preceded by a | so that the new "trimming" version is matched first. If there is no hyphen, then the original regex is used.
  • The reason \n is optional is so that {%- and {{- still work at the beginning of a template, and -%} and -}} still work at the end of a template.
  • The reason for the (?: ... ) non-capturing group surrounding each regex is that they are included in the PartialTemplateParser regex, whose behavior would be changed (read: broken) by the new |.
  • Without changing the code and running new and existing tests, I'm not sure whether VariableIncompleteEnd or AnyStartingTag would need to change as well.

It seems this new syntax would be better than a configuration option, because this way gives designers 100% control over the output regardless of their environment, rather than being at the mercy of how the current Liquid environment is configured. In other words, in my opinion, the language features of Liquid in app A should be exactly the same as they are in app B. Besides custom tags and filters, Liquid code should be portable.

I also think this would be better than what is implemented #171 because (a) it handles pre-trim and post-trim, and (b) it has a slightly smaller footprint in terms of code changes.

I'll try this out when I am able to and will submit a pull request if everything checks out. Until then, does anyone have any input on why this might be a bad idea (other than performance, which I'll test) or on anything I might be overlooking?

@fw42
Copy link
Copy Markdown
Contributor

fw42 commented Jun 16, 2013

Go for it, Nick! We were actually talking about implementing something like this for Shopify the other day, because lots of people use complex Liquid which literally generates pages with 10 megabytes of whitespace in it :-(

@fw42 fw42 mentioned this pull request Jun 26, 2013
@fw42
Copy link
Copy Markdown
Contributor

fw42 commented Jul 3, 2013

See #215.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants