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

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

[coveralls]

Coverage decreased (-39.8%) to 49.374% when pulling 0b920bd80a5dcd594858356ed774859a913b3ef2 on sgargan:sgargan/whitespace-handling into e0ae1590bfe2584ce1146f3238f0a4f055b7fd3f 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 [email protected] 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 https://github.com/osteele/liquid/pull/36#issuecomment-491619337, 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?