harttle / liquidjs

A simple, expressive, safe and Shopify compatible template engine in pure JavaScript.
https://liquidjs.com
MIT License
1.51k stars 240 forks source link

trim_right too greedy #19

Closed ilg-ul closed 7 years ago

ilg-ul commented 7 years ago

It looks like the trim_right option also removes white spaces from the next line.

For example:

    // {{ register.name }} bitfields.
{% for field in register.fields %}
    state->u.{{ deviceFamily | downcase }}.fld.{{ register.name | downcase | c_reserved }}.{{ field.name | downcase | c_reserved }} = cm_object_get_child_by_name(state->u.{{ deviceFamily | downcase }}.reg.{{ register.name | downcase | c_reserved }}, "{{ field.name }}"); // {{ field.bitOffset }}, {{ field.bitWidth }}.
{% endfor %} 

will generate:

// SRbitfields.
state->u.f4.fld.sr.awd= cm_object_get_child_by_name(state->u.f4.reg.sr, "AWD"); // 0, 1.
state->u.f4.fld.sr.eoc= cm_object_get_child_by_name(state->u.f4.reg.sr, "EOC"); // 1, 1.
state->u.f4.fld.sr.jeoc= cm_object_get_child_by_name(state->u.f4.reg.sr, "JEOC"); // 2, 1.
state->u.f4.fld.sr.jstrt= cm_object_get_child_by_name(state->u.f4.reg.sr, "JSTRT"); // 3, 1.
state->u.f4.fld.sr.strt= cm_object_get_child_by_name(state->u.f4.reg.sr, "STRT"); // 4, 1.
state->u.f4.fld.sr.ovr= cm_object_get_child_by_name(state->u.f4.reg.sr, "OVR"); // 5, 1.
// CR1bitfields.
state->u.f4.fld.cr1.awdch= cm_object_get_child_by_name(state->u.f4.reg.cr1, "AWDCH"); // 0, 5.

There are several problems here:

The second and third problems seems to indicate that the trim_right default should not apply to {{ ... }}, but only to tags.

In other implementations the white space control logic seems more elaborate (for example http://jinja.pocoo.org/docs/dev/templates/#whitespace-control).

harttle commented 7 years ago

You're right, I'm also find it too greedy to use. But I've tested with the Ruby version and it also trims the spaces after (or before) the \n. So let's consider the jinja fashioned trim.

The only thing I'm not sure is, the trim rules would be more sophisticated if we apply the more elaborate control. Consider if we have nested tags all with indentation:

<div>
    {% for xxx %}
        {% if xxx %}
            foo
        {% endif %}
    {% endfor %}
</div>

Now what I want is the line which actually rendered (i.e. foo) be indented by ONE level rather than three. The Ruby implementation (as the same with the currently implementation of shopify-liquid) is able to handle this by apply trim rules manually:

<div>
    {% for xxx -%}
        {%- if xxx -%}
            foo
        {%- endif -%}
    {%- endfor %}
</div>

Note that we just leave off the first begin tag and last end tag. This is a little trivial, but it works perfectly. I'm not sure if it also works if we apply the same behavior with jinja.

ilg-ul commented 7 years ago

I don't know how other implementations behave, but in my case I gave up using the global flags to be sure the trimming does not apply to {{ }}, and tried to use -%}.

Unfortunately this was also problematic, in my opinion because the trimming did not stop on the current line, but went on and removed spaces from the next line.

So finally I reverted to the original syntax, which merges tags and content on a single line:

{% if condition %}   // Some text{% endif %}

The desired syntax would be:

{% if condition %}
   // Some text
{% endif %}

The alternate syntax would be:

{% if condition -%}
   // Some text
{% endif -%}

Suggestions:

harttle commented 7 years ago

I made a commit to fix this behavior. Basically:

To be clear, the following code:

{%- if true -%}
    blabla
{%- endif -%}

will be rendered as:

    blabla

And {{- "hello" -}} world will be rendered as:

hello world

This commit will be released as a minor (instead of a patch) version since it'll potentially break current dependents.

ilg-ul commented 7 years ago

it is better than before, but it still behaves unexpectedly.

for example:

          // {{ register.name }} ({{ register.description }}) bitfields.
          struct { 
{% if register.fields %}
{% for field in register.fields %}
            Object *{{ field.name | downcase | c_reserved }}; // [{{ field.bitOffset }}:{{ field.bitOffset | plus: field.bitWidth | minus: 1 }}] {{ field.description }}
{% endfor %} 
{% endif %}
          } {{ register.name | downcase | c_reserved }}; 
{% endfor %}
        } fld;
      } {{ deviceFamily | downcase }};

with trim_right: true, concatenates all Object definitions on a single line.

the workaround I found was to add an empty line:

          // {{ register.name }} ({{ register.description }}) bitfields.
          struct { 
{% if register.fields %}
{% for field in register.fields %}
            Object *{{ field.name | downcase | c_reserved }}; // [{{ field.bitOffset }}:{{ field.bitOffset | plus: field.bitWidth | minus: 1 }}] {{ field.description }}

{% endfor %} 
{% endif %}
          } {{ register.name | downcase | c_reserved }}; 
{% endfor %}
        } fld;
      } {{ deviceFamily | downcase }};

but I think this is not ok.

frankly, I'm no longer able to follow the logic of whitespace trimming... :-(

harttle commented 7 years ago

Thank you for your suggestions and patience :)

I think you're right, {{ }} should not be affected since they're inline statements. Still there's one more thing I need your suggestion: should the {%- tag trim the \n character before?

On the one hand, we may need the following

<li>
{% if true %}
xxx
{% endif %}
</li>

to be rendered as <li>xxx</li>, thus both the {% if and {% endif need to trim the leading \n. On the other hand, we may need this

{% for field in register.fields %}
            Object xxx
{% endfor %}

to be rendered as

            Object xxx
            Object xxx
            Object xxx

thus the {% endfor %} need to maintain the \n after xxx.

ilg-ul commented 7 years ago

should the {%- tag trim the \n character before?

nope

to be rendered as

<li>xxx</li>

then I would write it

<li>{% if true %}xxx{% endif %}</li>
harttle commented 7 years ago

Agreed. Then the -%} still need to trim the trailing \n, this is useful for nested tags:

{% for .. %}
  {% if xx %}
    xxx
  {% endif %}
{% endfor %}

need to be rendered without double \n before and after.

ilg-ul commented 7 years ago

but the xxx must have the \n preserved.

harttle commented 7 years ago

implemented as above, released @1.7.1

ilg-ul commented 7 years ago

seems ok, so far.

thank you.

asah-asah commented 3 years ago

Hi @ilg-ul

Could you please tell me how did you generate the following code and what is it's input?

// SRbitfields. state->u.f4.fld.sr.awd= cm_object_get_child_by_name(state->u.f4.reg.sr, "AWD"); // 0, 1. state->u.f4.fld.sr.eoc= cm_object_get_child_by_name(state->u.f4.reg.sr, "EOC"); // 1, 1. state->u.f4.fld.sr.jeoc= cm_object_get_child_by_name(state->u.f4.reg.sr, "JEOC"); // 2, 1. state->u.f4.fld.sr.jstrt= cm_object_get_child_by_name(state->u.f4.reg.sr, "JSTRT"); // 3, 1. state->u.f4.fld.sr.strt= cm_object_get_child_by_name(state->u.f4.reg.sr, "STRT"); // 4, 1. state->u.f4.fld.sr.ovr= cm_object_get_child_by_name(state->u.f4.reg.sr, "OVR"); // 5, 1. // CR1bitfields. state->u.f4.fld.cr1.awdch= cm_object_get_child_by_name(state->u.f4.reg.cr1, "AWDCH"); // 0, 5.

I need to generate similar one for STM32H7 series(file attached: modified the extension as .txt for uploading) usart.c.txt

. Would be of great help if you can let me know the procedure and tools used. Thanks in advance!

ilg-ul commented 3 years ago

@harttle, please remove this misplaced question.