Modify whitespace trimming to be less hungry

[sgargan]

The liquid spec is a little vague about quite how greedy whitespace
trimming should be. Currently handling here will eat any available
whitespace between tags including all newline characters
e.g.

(

{%- sometag -%}

)

will render as

()

This is ok for web markup where the whitespace is not important, but for
other document types this greedy consumption is problematic.

Ideally the ws trimming would be less hungry and only trim any whitespace
on the line containing the tag up to and including it's newline, essentially
removing any trace of the tag but leaving other lines intact rendering the above as

(

)

The change here detects whitespace as tokens as they are parsed and
discards any as indicated by the trimming directives up to an including
the first newline.

Checklist

• I have read the contribution guidelines.
• make test passes.
• make lint passes.
• New and changed code is covered by tests.
• Performance improvements include benchmarks.
• Changes match the documented (not just the implemented) behavior of Shopify.

[coveralls]

Coverage decreased (-39.8%) to 49.374% when pulling 0b920bd on sgargan:sgargan/whitespace-handling into e0ae159 on osteele:master.

[sgargan]

Coverage decrease reported here is not correct ^

[osteele]

I'll look at the coverage.

Do you happen to know how this tracks the reference (Ruby) implementation of Liquid? Does it make it more or less like Ruby Liquid?

(I'm planning to accept it either way, but if it makes this package act less like Ruby Liquid then it probably needs a configuration option. Otherwise an option isn't necessary and probably needlessly increases the complexity.)

[sgargan]

I've not really played with the ruby version, but will take a look and see how it handles whitespace. I'd considered adding a config option but then realized you can still end up with the same whitespace as before by just not adding the extra new lines in the template, figured this was simpler than configuration and documentation. If you'd prefer a config option I can look into it. Any idea why the coverage is reporting so differently?

…

On Sun, May 12, 2019 at 7:46 PM Oliver Steele ***@***.***> wrote: I'll look at the coverage. Do you happen to know how this tracks the reference (Ruby) implementation of Liquid? Does it make it more or less like Ruby Liquid? (I'm planning to accept it either way, but if it makes this passage act *less* like Ruby Liquid then it probably needs a configuration option. Otherwise an option isn't necessary and probably needlessly increases the complexity.) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#36 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAA7RYPRQDSRCZM7QVDTYN3PVBQZLANCNFSM4HHIORSQ> .

[bglw]

Just jumping into this old thread to say that this behaviour wouldn't match the ruby liquid implementation, nor the liquidjs implementation. Both trim out newlines alongside whitespace. If merged it should certainly not be the default option.

[osteele]

@sgargan can the whitespace control options be used to do what you want?