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 smartify Liquid filter for SmartyPants #4323
Conversation
This is a tough spot, I don't know how @jekyll/core likes file organization in that aspect. For my own libs and apps, I would put it in |
How does the SmartyPants converter hook into |
Hm. Ok. Can it point to a class in our namespace? I'm generally weary of writing something in someone else's namespace. |
Yes, good call. How does this look? The failing tests freaked me out since they are related to Kramdown, but they are the same tests that are failing on master, so... |
There is one test that fails on Ruby 2.0 and nothing else:
Any idea what could be tripping up Ruby 2.0? |
According to Kramdown:
It seems to work anyway, but maybe this is the cause of the Ruby 2.0 fail? |
Ah, very interesting. So I guess we do. If it's that small, I don't mind it being in the smartypants converter. |
@@ -172,6 +172,7 @@ def sanitized_path(base_directory, questionable_path) | |||
require_all 'jekyll/commands' | |||
require_all 'jekyll/converters' | |||
require_all 'jekyll/converters/markdown' | |||
require_all 'jekyll/converters/smartypants' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be covered by line 173 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 173 included jekyll/converters/smartypants.rb
but not jekyll/converters/smartypants/kramdown_parser.rb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think both those requires should be put into jekyll/converters
tbh, they are subs of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One is a sub of Kramdown::Parser::Markdown
, another of Jekyll::Converter
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@parkr wat? I'm talking about the markdown and smartypants requires, both those belong in jekyll/coverters.rb.
In the same file? |
Yeah. It's like 5 lines, right? |
I have added several tests, and am now allowing HTML to passthrough. There are separate filters that will escape/strip HTML, if that is desired. |
Just for reference, I brought up the idea of adding this parser to kramdown proper. |
@jekyllbot: merge +minor |
Most useful robot EVER. |
Would definitely love an alternative to |
Is there a better place to put the
Kramdown::Parser::SmartyPants
class?Am I on the right track with this?