New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add whitespace control character and associated tests #773

Merged
merged 2 commits into from Aug 11, 2016

Conversation

Projects
None yet
6 participants
@evulse
Copy link
Contributor

evulse commented Jun 27, 2016

This adds support for {{- and {%- syntax which will lstrip! and -}} and -%} which will rstrip!

Depends on Shopify/liquid-c#30 to pass all tests.

Resolve issues #216, #215, #214, #194, #171, #162

Previously Pull Request #746

@evulse

This comment has been minimized.

Copy link
Contributor Author

evulse commented Jun 27, 2016

@fw42 was easier to regenerate pull request on new fork

@@ -48,6 +54,17 @@ def parse(tokenizer, parse_context)
yield nil, nil
end

def whitespace_handler(token, parse_context)
if token[2] == WhitespaceControl
previous_token = @nodelist.pop

This comment has been minimized.

@dylanahsmith

dylanahsmith Jun 30, 2016

Member

Won't this pop a node from the nodelist even if the previous token wasn't a string?

This comment has been minimized.

@pushrax

pushrax Jul 1, 2016

Member

Should use .last as in the C branch.

@dylanahsmith

This comment has been minimized.

Copy link
Member

dylanahsmith commented Jul 12, 2016

LGTM

@fw42

This comment has been minimized.

Copy link
Member

fw42 commented Aug 5, 2016

The test fails here are because the change in liquid-c isn't merged yet, right?

@evulse

This comment has been minimized.

Copy link
Contributor Author

evulse commented Aug 5, 2016

Yes as soon as this is pointed to the new liquid-c commit these pass as expected. Also the liquid-c version passes with this commit included as well.

@evulse

This comment has been minimized.

Copy link
Contributor Author

evulse commented Aug 6, 2016

screen shot 2016-08-06 at 11 49 19 pm

Just using Mozilla's Nunjucks on a project and noticed it uses this same syntax. Good to see all the libraries heading in this direction.

@fw42 fw42 merged commit f251856 into Shopify:master Aug 11, 2016

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

@fw42 fw42 referenced this pull request Aug 11, 2016

Closed

Whitespace issues #216

@fw42

This comment has been minimized.

Copy link
Member

fw42 commented Aug 11, 2016

Thanks for your contribution, great job!

@parkr

This comment has been minimized.

Copy link
Contributor

parkr commented Aug 11, 2016

Liquid 4?!?!?! Could this be a thing in Liquid 4?!?!

@parkr

This comment has been minimized.

Copy link
Contributor

parkr commented Aug 11, 2016

💗 🎉 🎊

@fw42

This comment has been minimized.

Copy link
Member

fw42 commented Aug 11, 2016

I don't see why not

@galopin

This comment has been minimized.

Copy link

galopin commented Aug 30, 2016

@evulse @fw42 Wondering if this nifty whitespace control feature could be backported into the 3.x version ⁉️

@pathawks pathawks referenced this pull request Oct 20, 2016

Merged

Output plugins in feed #8

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