Skip to content
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

Liquid: several fixes and additions #1327

Merged
merged 21 commits into from
Sep 10, 2019

Conversation

EricFromCanada
Copy link
Contributor

I set out to fix this page's highlighting error, and it snowballed from there.

Support has been added for several Liquid features that probably weren't present when this lexer was first written, plus a few expected in the next release. I also refactored for simplicity and accuracy. Left for a future PR is support for the upcoming {% liquid %} tag.

I've left each change as separate commits so you can see what I did, and can squash if needed.

"and" & "or" are operators, but "not" & "!" are not.
Both can be assigned to create an empty string, but only `empty` is equivalent to an empty string or array.
https://github.com/Shopify/liquid/blob/master/test/integration/tags/statements_test.rb
Removed several redundant lines and states, integrated :array_index into :variable, minor fixes, code style cleanup
@pyrmont pyrmont self-assigned this Sep 9, 2019
@pyrmont pyrmont added the needs-review The PR needs to be reviewed label Sep 9, 2019
@pyrmont
Copy link
Contributor

pyrmont commented Sep 10, 2019

@EricFromCanada This looks like a great set of improvements to the lexer!

I had two minor suggestions as set out in the two commits. Let me know what you think.

@pyrmont pyrmont added author-action The PR has been reviewed but action by the author is needed and removed needs-review The PR needs to be reviewed labels Sep 10, 2019
@pyrmont
Copy link
Contributor

pyrmont commented Sep 10, 2019

@EricFromCanada Oh, and I see part of the impetus for this PR is to improve the official Liquid documentation.

It's worth noting that unfortunately the improvements you've submitted here won't show up in sites that use GitHub Pages (which the Liquid docs do). This is because GitHub has locked the version of Rouge used in GitHub Pages to version 2.2.1. There is an open issue about this but I'm not sure if/when it will be addressed. (We still appreciate the fixes!)

@EricFromCanada
Copy link
Contributor Author

Looks good to me. Once this is merged and released I'll poke github/pages-gem#652 to get updated. My hope is that this'll be one more reason for them to bump the gem to rouge 3.x, since it'll benefit both the jekyll & liquid docs.

@pyrmont pyrmont merged commit e3f26ee into rouge-ruby:master Sep 10, 2019
@pyrmont pyrmont removed the author-action The PR has been reviewed but action by the author is needed label Sep 10, 2019
@pyrmont
Copy link
Contributor

pyrmont commented Sep 10, 2019

@EricFromCanada Thanks so much for this contribution. We're currently on a two-week release cadence as we clear the PR backlog. The updated lexer will be part of v3.11.0 of Rouge and is scheduled to be pushed out on 17 September.

@EricFromCanada EricFromCanada deleted the liquid-fixes branch September 10, 2019 19:42
@ashmaroli
Copy link
Contributor

@EricFromCanada Thank you for the fixes and enhancements.
Any reason why you left {%, {{, |, }}, %} to continue being tokenized as Punctuation even though they have a fixed, special meaning in Liquid..?

@EricFromCanada
Copy link
Contributor Author

@ashmaroli It never occurred to me at the time, but I suppose a case could be made to mark them as Comment.Preproc in a subsequent PR.

@ashmaroli
Copy link
Contributor

@EricFromCanada Good to know. While I'm not sure on whether they ought to be Comment.Preproc or Keyword.Pseudo, let us defer that discussion for the potential pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants