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 to LIQUID_TAG_REGEX #7015

Merged
merged 6 commits into from May 17, 2018

Conversation

Projects
None yet
4 participants
@kylebarbour
Contributor

kylebarbour commented May 15, 2018

Fixes #7009. Simply adds -? to the beginning and end of the regexes used in /lib/jekyll/excerpt.rb to allow for Liquid whitespace control characters.

@pathawks

This comment has been minimized.

Member

pathawks commented May 15, 2018

Oh sweet! Thanks for catching this @kylebarbour 👍

@ashmaroli Does LIQUID_TAG_REGEX have any tests? Does this PR need to add any, or maybe we should make a note to tackle that later?

@pathawks pathawks added the fix label May 15, 2018

@pathawks pathawks requested review from ashmaroli and jekyll/build May 15, 2018

@ashmaroli

This comment has been minimized.

Member

ashmaroli commented May 15, 2018

@kylebarbour Thank you for opening the PR.
The regex changes look good. This does need a test too though.

You may refer to #6724 to see what tests are currently in place to test this functionality. If it looks confusing to you, the following note should guide you:

All you need to do is (at minimum):

  • add two new posts in the test/source/_posts directory, each containing Liquid Block-type tag with the whitespace_control character.
    One post (test subject) will contain only the "opening" construct as part of the excerpt. (The "closing" construct is to be written after a blank line). The other post (control subject) will contain the same content but without the blank line before the "closing" construct.
  • append assertions to test/test_excerpt.rb using the above two posts.
  • adjust test/test_generated_site.rb.

Run script/test test/test_excerpt.rb to see if your patch passes the base test.

(Optional): Once you're comfortable with the tests, you can add and test more posts with various valid placement of the whitespace-control character.

If you still find it hard to add tests for this PR, feel free to inform so here.

@kylebarbour

This comment has been minimized.

Contributor

kylebarbour commented May 16, 2018

Thanks for guiding me through the process! I'll take a stab at it.

@kylebarbour

This comment has been minimized.

Contributor

kylebarbour commented May 16, 2018

@ashmaroli Take a look and see what you think — I think this all works!

@kylebarbour

This comment has been minimized.

Contributor

kylebarbour commented May 16, 2018

Added a more informative error message when something resembling a Liquid tag is found but doesn't have superclass Liquid::Block.

@kylebarbour kylebarbour reopened this May 16, 2018

@ashmaroli

This comment has been minimized.

Member

ashmaroli commented May 16, 2018

@kylebarbour The tests look good. 👍

The message to Jekyll Logger converts all "whitespace" to a "single space" character. Therefore, Jekyll.logger.error "Error:", "foo\nbar" will simply output Error: foo bar..

So, IMO this should be in 2 separate calls:

Jekyll.logger.error "Error:",
  "This page's excerpt appears to contain a Liquid tag which couldn't be parsed"
Jekyll.logger.error "", e.message

Disclaimer: The suggestion above has not been tested locally..

Since, I can't test locally, I'm not sure if Jekyll has to log e.message at all, since we're going to be calling raise with the same message..

@kylebarbour

This comment has been minimized.

Contributor

kylebarbour commented May 16, 2018

Great! You're right, logging e.message is redundant, so I've removed it.

Jekyll.logger.error "Error:",
"Excerpt in #{doc.relative_path} appears to contain a Liquid tag which " \
"couldn't be parsed."
raise

This comment has been minimized.

@pathawks

pathawks May 16, 2018

Member

This seems like a nice compromise to me for something like a 3.8.2: a much more helpful error message without any material change in behavior 👍

This comment has been minimized.

@ashmaroli

ashmaroli May 17, 2018

Member

Can we shorten the message a bit..? :

Jekyll.logger.error "Error:",
  "Liquid tag in #{doc.relative_path} couldn't be parsed."

This comment has been minimized.

@pathawks

pathawks May 17, 2018

Member

Should mention “excerpt” to hopefully narrow things down for the user.

This comment has been minimized.

@ashmaroli

ashmaroli May 17, 2018

Member

@pathawks "excerpt" will be in the path because the error arises only for Excerpt instances. For example:

Liquid tag in _members/john-doe.md/#excerpt couldn't be parsed.

This comment has been minimized.

@pathawks

pathawks May 17, 2018

Member

I would favor a more descriptive error message over a less descriptive one, but if you think this is sufficient, 👍

This comment has been minimized.

@kylebarbour

kylebarbour May 17, 2018

Contributor

I've written a slightly more succinct error message — what do you think?

@ashmaroli I don't think the word excerpt will necessarily be in the path, as it wasn't in my original error that mentioned the superclass (see my comment in #7009). The excerpts seem to be generated for everything whether they're needed or not; I get about 20 ms of performance improvement from commenting out generate_excerpt in lib/jekyll/document.rb.

Running jekyll build on my test case without the custom error message, all that I get is

Error: could not read file <file>: undefined method `superclass' for nil:NilClass

without anything about excerpts.

This comment has been minimized.

@ashmaroli

ashmaroli May 17, 2018

Member

This is the reason why we decided to add a helpful error message.. 😉

@ashmaroli ashmaroli removed the needs-tests label May 17, 2018

Kyle Barbour
rescue NoMethodError
Jekyll.logger.error "Error:",
"A Liquid tag in the excerpt of #{doc.relative_path} couldn't be " \
"parsed."

This comment has been minimized.

@pathawks

pathawks May 17, 2018

Member

This makes a lot of sense to me 👍

@ashmaroli

Lets not have a trivial log message block this.. All changes LGTM!

@pathawks

This comment has been minimized.

Member

pathawks commented May 17, 2018

@jekyllbot: merge +fix

@jekyllbot jekyllbot merged commit b915c75 into jekyll:master May 17, 2018

3 checks passed

WIP ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jekyllbot jekyllbot added bug fix labels May 17, 2018

@kylebarbour

This comment has been minimized.

Contributor

kylebarbour commented May 17, 2018

Thanks all! You were really helpful and fun to work with — as a truly rookie programmer I appreciate it.

@kylebarbour kylebarbour deleted the kylebarbour:excerpt-regex branch May 17, 2018

@pathawks

This comment has been minimized.

Member

pathawks commented May 17, 2018

@kylebarbour Thanks so much for all your work on this. Hoping this won't be your last contribution 😉🍻

@ashmaroli

This comment has been minimized.

Member

ashmaroli commented May 18, 2018

@pathawks Would you mind handling the backport of this patch for a v3.8.2..?

pathawks added a commit that referenced this pull request May 18, 2018

pathawks added a commit that referenced this pull request May 18, 2018

pathawks added a commit that referenced this pull request May 18, 2018

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