Add smartify Liquid filter for SmartyPants #4323

Merged
merged 2 commits into from Jan 10, 2016

Conversation

Projects
None yet
5 participants
@pathawks
Member

pathawks commented Jan 7, 2016

Is there a better place to put the Kramdown::Parser::SmartyPants class?

Am I on the right track with this?

@pathawks

This comment has been minimized.

Show comment
Hide comment
Member

pathawks commented Jan 7, 2016

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jan 7, 2016

Contributor

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 lib/kramdown/parser/smarty_pants even if I had lib/jekyll but not all authors are strict in that way in that the class must point to the file or I'm doing it wrong.

Contributor

envygeeks commented Jan 7, 2016

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 lib/kramdown/parser/smarty_pants even if I had lib/jekyll but not all authors are strict in that way in that the class must point to the file or I'm doing it wrong.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 7, 2016

Member

How does the SmartyPants converter hook into Kramdown::Parser::SmartyPants?

Member

parkr commented Jan 7, 2016

How does the SmartyPants converter hook into Kramdown::Parser::SmartyPants?

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 8, 2016

Member

Hm. Ok. Can it point to a class in our namespace? I'm generally weary of writing something in someone else's namespace.

Member

parkr commented Jan 8, 2016

Hm. Ok. Can it point to a class in our namespace? I'm generally weary of writing something in someone else's namespace.

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Jan 8, 2016

Member

Can it point to a class in our 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...

Member

pathawks commented Jan 8, 2016

Can it point to a class in our 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...

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Jan 8, 2016

Member

There is one test that fails on Ruby 2.0 and nothing else:

Error:
TestFilters#test_: filters should smartify with simple string. :
NameError: wrong constant name Jekyll::Converters::SmartyPants::KramdownParser
    /home/travis/build/jekyll/jekyll/vendor/bundle/ruby/2.0.0/gems/kramdown-1.9.0/lib/kramdown/document.rb:106:in `const_defined?'
    /home/travis/build/jekyll/jekyll/vendor/bundle/ruby/2.0.0/gems/kramdown-1.9.0/lib/kramdown/document.rb:106:in `initialize'
    /home/travis/build/jekyll/jekyll/lib/jekyll/converters/smartypants.rb:23:in `new'
    /home/travis/build/jekyll/jekyll/lib/jekyll/converters/smartypants.rb:23:in `convert'
    /home/travis/build/jekyll/jekyll/lib/jekyll/filters.rb:26:in `smartify'
    /home/travis/build/jekyll/jekyll/test/test_filters.rb:35:in `block (2 levels) in <class:TestFilters>'
    /home/travis/build/jekyll/jekyll/vendor/bundle/ruby/2.0.0/gems/shoulda-context-1.2.1/lib/shoulda/context/context.rb:413:in `instance_exec'
    /home/travis/build/jekyll/jekyll/vendor/bundle/ruby/2.0.0/gems/shoulda-context-1.2.1/lib/shoulda/context/context.rb:413:in `block in create_test_from_should_hash'

Any idea what could be tripping up Ruby 2.0?

Member

pathawks commented Jan 8, 2016

There is one test that fails on Ruby 2.0 and nothing else:

Error:
TestFilters#test_: filters should smartify with simple string. :
NameError: wrong constant name Jekyll::Converters::SmartyPants::KramdownParser
    /home/travis/build/jekyll/jekyll/vendor/bundle/ruby/2.0.0/gems/kramdown-1.9.0/lib/kramdown/document.rb:106:in `const_defined?'
    /home/travis/build/jekyll/jekyll/vendor/bundle/ruby/2.0.0/gems/kramdown-1.9.0/lib/kramdown/document.rb:106:in `initialize'
    /home/travis/build/jekyll/jekyll/lib/jekyll/converters/smartypants.rb:23:in `new'
    /home/travis/build/jekyll/jekyll/lib/jekyll/converters/smartypants.rb:23:in `convert'
    /home/travis/build/jekyll/jekyll/lib/jekyll/filters.rb:26:in `smartify'
    /home/travis/build/jekyll/jekyll/test/test_filters.rb:35:in `block (2 levels) in <class:TestFilters>'
    /home/travis/build/jekyll/jekyll/vendor/bundle/ruby/2.0.0/gems/shoulda-context-1.2.1/lib/shoulda/context/context.rb:413:in `instance_exec'
    /home/travis/build/jekyll/jekyll/vendor/bundle/ruby/2.0.0/gems/shoulda-context-1.2.1/lib/shoulda/context/context.rb:413:in `block in create_test_from_should_hash'

Any idea what could be tripping up Ruby 2.0?

@pathawks pathawks referenced this pull request in jekyll/jekyll-seo-tag Jan 8, 2016

Closed

Add smartify filter #19

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Jan 8, 2016

Member

Can it point to a class in our namespace? I'm generally weary of writing something in someone else's namespace.

According to Kramdown:

The special options key :input can be used to select the parser that should parse the source. It has to be the name of a class in the Kramdown::Parser module.

It seems to work anyway, but maybe this is the cause of the Ruby 2.0 fail?

Member

pathawks commented Jan 8, 2016

Can it point to a class in our namespace? I'm generally weary of writing something in someone else's namespace.

According to Kramdown:

The special options key :input can be used to select the parser that should parse the source. It has to be the name of a class in the Kramdown::Parser module.

It seems to work anyway, but maybe this is the cause of the Ruby 2.0 fail?

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 8, 2016

Member

The special options key :input can be used to select the parser that should parse the source. It has to be the name of a class in the Kramdown::Parser module.

Ah, very interesting. So I guess we do. If it's that small, I don't mind it being in the smartypants converter.

Member

parkr commented Jan 8, 2016

The special options key :input can be used to select the parser that should parse the source. It has to be the name of a class in the Kramdown::Parser module.

Ah, very interesting. So I guess we do. If it's that small, I don't mind it being in the smartypants converter.

@parkr

View changes

lib/jekyll.rb
@@ -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'

This comment has been minimized.

@parkr

parkr Jan 8, 2016

Member

this should be covered by line 173 :)

@parkr

parkr Jan 8, 2016

Member

this should be covered by line 173 :)

This comment has been minimized.

@pathawks

pathawks Jan 8, 2016

Member

Line 173 included jekyll/converters/smartypants.rb but not jekyll/converters/smartypants/kramdown_parser.rb

@pathawks

pathawks Jan 8, 2016

Member

Line 173 included jekyll/converters/smartypants.rb but not jekyll/converters/smartypants/kramdown_parser.rb

This comment has been minimized.

@envygeeks

envygeeks Jan 8, 2016

Contributor

I think both those requires should be put into jekyll/converters tbh, they are subs of it.

@envygeeks

envygeeks Jan 8, 2016

Contributor

I think both those requires should be put into jekyll/converters tbh, they are subs of it.

This comment has been minimized.

@parkr

parkr Jan 8, 2016

Member

One is a sub of Kramdown::Parser::Markdown, another of Jekyll::Converter?

@parkr

parkr Jan 8, 2016

Member

One is a sub of Kramdown::Parser::Markdown, another of Jekyll::Converter?

This comment has been minimized.

@envygeeks

envygeeks Jan 8, 2016

Contributor

@parkr wat? I'm talking about the markdown and smartypants requires, both those belong in jekyll/coverters.rb.

@envygeeks

envygeeks Jan 8, 2016

Contributor

@parkr wat? I'm talking about the markdown and smartypants requires, both those belong in jekyll/coverters.rb.

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Jan 8, 2016

Member

If it's that small, I don't mind it being in the smartypants converter.

In the same file?

Member

pathawks commented Jan 8, 2016

If it's that small, I don't mind it being in the smartypants converter.

In the same file?

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 8, 2016

Member

In the same file?

Yeah. It's like 5 lines, right?

Member

parkr commented Jan 8, 2016

In the same file?

Yeah. It's like 5 lines, right?

@parkr parkr added the Enhancement label Jan 8, 2016

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Jan 9, 2016

Member

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.

Member

pathawks commented Jan 9, 2016

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.

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Jan 10, 2016

Member

Just for reference, I brought up the idea of adding this parser to kramdown proper.

gettalong/kramdown#299

Member

pathawks commented Jan 10, 2016

Just for reference, I brought up the idea of adding this parser to kramdown proper.

gettalong/kramdown#299

@parkr parkr changed the title from Add smartify filter for SmartyPants to Add smartify Liquid filter for SmartyPants Jan 10, 2016

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 10, 2016

Member

@jekyllbot: merge +minor

Member

parkr commented Jan 10, 2016

@jekyllbot: merge +minor

jekyllbot added a commit that referenced this pull request Jan 10, 2016

@jekyllbot jekyllbot merged commit 15eaefa into jekyll:master Jan 10, 2016

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

jekyllbot added a commit that referenced this pull request Jan 10, 2016

@pathawks pathawks deleted the pathawks:smartypants branch Jan 10, 2016

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 10, 2016

Member

Most useful robot EVER.

Member

parkr commented Jan 10, 2016

Most useful robot EVER.

@ndarville

This comment has been minimized.

Show comment
Hide comment
@ndarville

ndarville Jan 11, 2016

Contributor

Would definitely love an alternative to {{ page.title | markdownify | strip_html }}. :)

Contributor

ndarville commented Jan 11, 2016

Would definitely love an alternative to {{ page.title | markdownify | strip_html }}. :)

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