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

Re-implement handling Liquid blocks in excerpts #7250

Merged
merged 5 commits into from Nov 4, 2018

Conversation

ashmaroli
Copy link
Member

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 a team September 16, 2018 07:02
@ashmaroli ashmaroli force-pushed the excerpt-liquid-block-patch branch 3 times, most recently from 6526f8f to d46fdd6 Compare September 16, 2018 08:01
@ghost ghost added priority 2 (should) ⏪ backport Changes will be merged into an older stable branch labels Sep 16, 2018
@ashmaroli ashmaroli added backport-candidate Consider for merge into an older stable branch and removed ⏪ backport Changes will be merged into an older stable branch labels Sep 16, 2018
Copy link
Member

@mattr- mattr- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for catching this @mattr- 🙂

lib/jekyll/excerpt.rb Outdated Show resolved Hide resolved
@ashmaroli
Copy link
Member Author

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-
Copy link
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?

@djacquel
Copy link

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

@ashmaroli
Copy link
Member Author

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
Copy link
Member Author

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

ref build time in seconds
#7250 344.02
master 329.06

@pathawks
Copy link
Member

@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."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

ghost pushed a commit that referenced this pull request Nov 4, 2018
@ghost
Copy link

ghost commented Nov 4, 2018

@jekyllbot: merge +bug

@jekyllbot jekyllbot merged commit 592b530 into jekyll:master Nov 4, 2018
jekyllbot added a commit that referenced this pull request Nov 4, 2018
@ashmaroli ashmaroli deleted the excerpt-liquid-block-patch branch November 4, 2018 19:07
ghost pushed a commit that referenced this pull request Nov 4, 2018
@ghost ghost mentioned this pull request Nov 4, 2018
ghost pushed a commit that referenced this pull request Nov 4, 2018
@jekyll jekyll locked and limited conversation to collaborators Nov 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Excerpt throws error when highlight and raw tags are combined at the begining of a document.
5 participants