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

Re-implement handling Liquid blocks in excerpts #7250

Merged
merged 5 commits into from Nov 4, 2018

Conversation

Projects
None yet
6 participants
@ashmaroli
Member

ashmaroli commented Sep 16, 2018

Resolves #7248

This re-implementation should be able to handle multiple Liquid blocks in excerpts.

@djacquel I invite you to test this branch against various valid combinations of Liquid blocks so that we can get rid of this issue for good. Thanks in advance.

@jekyll/core I request this PR to be considered a.s.a.p and be backported to the 3.8-stable series

@ashmaroli ashmaroli added the fix label Sep 16, 2018

@ashmaroli ashmaroli requested a review from jekyll/core Sep 16, 2018

@mattr-

Please address the inline comments.

As part of excerpt generator, do we need to be concerned about liquid object tag pairs ({{ and }})?

@@ -131,36 +131,44 @@ def render_with_liquid?
#
# Returns excerpt String
LIQUID_TAG_REGEX = %r!{%-?\s*(\w+).+\s*-?%}!m
LIQUID_TAG_REGEX = %r!{%-?\s*(\w+).+?\s*-?%}!m

This comment has been minimized.

@mattr-

mattr- Sep 16, 2018

Member

The capture for this regular expression isn't 100% correct. A liquid tag of {%highlight%} will lose the last letter, meaning that the capture group will contain highligh instead of highlight

I wrote a quick test case for this on Rubular as well, as an extra example: http://rubular.com/r/umPU5g5PiH

This comment has been minimized.

@ashmaroli

ashmaroli Sep 16, 2018

Member

Thank you for catching this @mattr- 🙂

Show resolved Hide resolved lib/jekyll/excerpt.rb Outdated
@ashmaroli

This comment has been minimized.

Member

ashmaroli commented Sep 16, 2018

do we need to be concerned about liquid object tag pairs ({{ and }})?

@mattr- No, we shouldn't handle deviation from the formats used by the majority of the users: {{variable}} or {{ variable }}.

@mattr-

This comment has been minimized.

Member

mattr- commented Sep 16, 2018

@ashmaroli Sorry, not sure I understood you. Are you saying that since {% and %} tag pairs are the most common, we don't need to handle {{ and }} tag pairs?

@mattr-

mattr- approved these changes Sep 16, 2018

@djacquel

This comment has been minimized.

djacquel commented Sep 16, 2018

No more error thrown, just the warning. Good job.

@ashmaroli

This comment has been minimized.

Member

ashmaroli commented Sep 16, 2018

Are you saying that since {% and %} tag pairs are the most common, we don't need to handle {{ and }} tag pairs?

@mattr- Not at all..
We are handling only {% and %} for Liquid::Block type tags. i.e. tags that have an opener
(e.g. {% raw %}) and a closer (e.g. {% endraw %}). Regular tags like {% assign %} are not handled.

@ashmaroli

This comment has been minimized.

Member

ashmaroli commented Sep 16, 2018

Oh.. The Utterson Report doesn't look good.. 🙁

ref build time in seconds
#7250 344.02
master 329.06
@pathawks

This comment has been minimized.

Member

pathawks commented Sep 16, 2018

@ashmaroli Correctness wins over performance 👍🏼

" #{doc.excerpt_separator.inspect}. "
Jekyll.logger.warn "", "The block has been modified with the appropriate closing tag."
Jekyll.logger.warn "", "Feel free to define a custom excerpt or excerpt_separator in the"
Jekyll.logger.warn "", "document's Front Matter if the generated excerpt is unsatisfactory."

This comment has been minimized.

@pathawks

pathawks Sep 17, 2018

Member

These are all the same warning, not separate messages, correct?

This comment has been minimized.

@ashmaroli

ashmaroli Sep 18, 2018

Member

@pathawks Yes. These calls are for the same warning.
I split them out into separate calls so that the messages appear to be formatted on the CLI.

I can't use a heredoc to emulate the formatting because Jekyll's logger squashes all whitespace into a single space char, which causes heredoc messages to simply print as a single line message.

ashmaroli added some commits Sep 18, 2018

base LIQUID_TAG_REGEX on upstream regex
ref: Liquid::BlockBody::FullToken
https://git.io/fAyaY
@oe

oe approved these changes Nov 4, 2018

oe added a commit that referenced this pull request Nov 4, 2018

@oe

This comment has been minimized.

Member

oe commented Nov 4, 2018

@jekyllbot: merge +bug

@jekyllbot jekyllbot merged commit 592b530 into jekyll:master Nov 4, 2018

1 of 4 checks passed

codeclimate 2 issues to fix
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
WIP ready for review
Details

@jekyllbot jekyllbot added the bug label Nov 4, 2018

@ashmaroli ashmaroli deleted the ashmaroli:excerpt-liquid-block-patch branch Nov 4, 2018

oe added a commit that referenced this pull request Nov 4, 2018

@oe oe referenced this pull request Nov 4, 2018

Merged

Backport #7250 to 3.8.x #7352

oe added a commit that referenced this pull request Nov 4, 2018

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