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

Supporting more kramdown options. #1201

Merged
merged 3 commits into from Jul 14, 2013

Conversation

Projects
None yet
6 participants
@zachgersh
Contributor

zachgersh commented Jun 10, 2013

Getting the conversation started on #1095. Taking @parkr's suggestion.

@zachgersh

This comment has been minimized.

Contributor

zachgersh commented Jun 11, 2013

@mattr- @parkr for review. I actually am assuming I should add some of the other kramdown options as tests? Additionally, this would probably require a docs update?

@mikegreiling

This comment has been minimized.

mikegreiling commented Jun 19, 2013

👍

@zachgersh

This comment has been minimized.

Contributor

zachgersh commented Jun 28, 2013

I am hoping you guys get some discussion in on this soon @parkr and @mattr-.

Unsure if anything else would actually be needed here.

def symbolize_keys!
keys.each do |key|
self[(key.to_sym rescue key) || key] = delete(key)

This comment has been minimized.

@mattr-

mattr- Jun 28, 2013

Member

What strange Ruby creature is this?

This comment has been minimized.

@zachgersh

zachgersh Jun 28, 2013

Contributor

Stolen as per @parkr's suggestion on #1095 :)

This comment has been minimized.

@parkr

parkr Jun 30, 2013

Member

Ripped from Rails core extensions. There's a note in my comment in #1095 :)

@zachgersh

This comment has been minimized.

Contributor

zachgersh commented Jun 28, 2013

So, the more you stare at the function the more awesome it becomes. The magic comes from delete(key) because it actually returns the value of what it deletes. So, returning the value from what we delete now sets the symbolized key to that value!

BRILLIANT!

@mattr-

This comment has been minimized.

Member

mattr- commented Jun 28, 2013

An additional test to verify passthrough of additional options plus a doc update would be nice to have here.

@zachgersh

This comment has been minimized.

Contributor

zachgersh commented Jun 28, 2013

Agreed,

Here's the issue I have though. For my test do I need to pass every single possible kramdown item in the hash? We allow all of them so it only seems right. What do you think @mattr-?

@mattr-

This comment has been minimized.

Member

mattr- commented Jun 29, 2013

I don't think we need every possible option. Ultimately, we just need to verify the they're in the hash properly. It's kramdown's responsibility to handle its own options.

On Fri, Jun 28, 2013 at 6:07 PM, Zach notifications@github.com wrote:

Agreed,

Here's the issue I have though. For my test do I need to pass every single possible kramdown item in the hash? We allow all of them so it only seems right. What do you think @mattr-?

Reply to this email directly or view it on GitHub:
#1201 (comment)

@zachgersh

This comment has been minimized.

Contributor

zachgersh commented Jun 30, 2013

sure thing @mattr- I will add a test today. @parkr any thoughts?

@maul-esel

View changes

lib/jekyll/converters/markdown/kramdown_parser.rb Outdated
@@ -26,7 +26,7 @@ def convert(content)
# not using coderay
base_kramdown_configs
end
Kramdown::Document.new(content, kramdown_configs).to_html
Kramdown::Document.new(content, @config["kramdown"].symbolize_keys).to_html

This comment has been minimized.

@maul-esel

maul-esel Jun 30, 2013

Contributor

You could delete all the other code in that method and the base_kramdown_configs method then, couldn't you?

This comment has been minimized.

@maul-esel

maul-esel Jun 30, 2013

Contributor

Looking at it again, will this work with the nested config hash (coderay) ?

This comment has been minimized.

@parkr

parkr Jun 30, 2013

Member

Yeah, it'd have to deep-symbolize if that is needed.

This comment has been minimized.

@parkr

parkr Jun 30, 2013

Member

Hm, where are coderay options used in kramdown? http://kramdown.rubyforge.org/options.html

This comment has been minimized.

@maul-esel

maul-esel Jun 30, 2013

Contributor

Hm, where are coderay options used in kramdown?

Kramdown wants the coderay options directly in its option hash (with symbol keys, of course), but jekyll seems to have historically chosen to have them in a nested coderay hash in _config.yml.

This comment has been minimized.

@parkr

parkr Jul 1, 2013

Member

I was wondering where in the Kramdown documentation these Coderay options are mentioned and why Kramdown wants them in their array of options.

This comment has been minimized.

@maul-esel

maul-esel Jul 1, 2013

Contributor

where in the Kramdown documentation these Coderay options are mentioned

On the site you linked, starting from option 3, coderay_bold_every.

I don't know Kramdown but it seems like another code highlighting system:

enable_coderay
Use coderay for syntax highlighting

This comment has been minimized.

@parkr

parkr Jul 1, 2013

Member

I'm literally blind. Sorry about that :)

@zachgersh

This comment has been minimized.

Contributor

zachgersh commented Jul 1, 2013

So,

Here is what I am going to do gents:

  1. I am going to write a couple of tests (symbolizing the keys and having coderay be part of the options hash)
  2. I am going to go ahead and remove the base_kramdown_configs methods
  3. I am going to make coderay part of the kramdown options hash

Then I will run the test suite (lol @ my spelling mistake) again to see if anything has broken.

Sound good?

@parkr

This comment has been minimized.

Member

parkr commented Jul 1, 2013

Sounds good to me. Thanks!

@mattr-

This comment has been minimized.

Member

mattr- commented Jul 1, 2013

Yup. 👍

@parkr

This comment has been minimized.

Member

parkr commented Jul 12, 2013

@zachgersh I'd love to include this in the v1.1 release if you have time today or tomorrow to finish this up?

@zachgersh

This comment has been minimized.

Contributor

zachgersh commented Jul 12, 2013

I will have this done for you tomorrow! Apologies for the delay.

On Jul 12, 2013, at 11:31 AM, Parker Moore notifications@github.com wrote:

@zachgersh I'd love to include this in the v1.1 release if you have time today or tomorrow to finish this up?


Reply to this email directly or view it on GitHub.

@parkr

This comment has been minimized.

Member

parkr commented Jul 12, 2013

No problem! :D

@parkr

This comment has been minimized.

Member

parkr commented Jul 13, 2013

@zachgersh Wait, why are we removing the base_kramdown_configs method?? I like to see those things separate - easier reading usually :)

@zachgersh

This comment has been minimized.

Contributor

zachgersh commented Jul 13, 2013

Okay. I will leave it in.

On Jul 13, 2013, at 9:36 AM, Parker Moore notifications@github.com wrote:

@zachgersh Wait, why are we removing the base_kramdown_configs method?? I like to see those things separate - easier reading usually :)


Reply to this email directly or view it on GitHub.

@parkr

This comment has been minimized.

Member

parkr commented Jul 13, 2013

If it's poor organizing then we can move thing around but if not, it would be wonderful if you left it in. Thank you!! ❤️

@maul-esel

This comment has been minimized.

Contributor

maul-esel commented Jul 13, 2013

It seems it is not used anymore in the code for this PR, that's why I had suggested so.

@maul-esel

This comment has been minimized.

Contributor

maul-esel commented Jul 13, 2013

I mean, it's called twice, but the result (kramdown_configs) is discarded anyway.

@zachgersh

This comment has been minimized.

Contributor

zachgersh commented Jul 13, 2013

Hey @maul-esel could you jump in the irc channel? I actually wanted to run a test by you. I discarded it without ill effect including the coderay bit as well.

@parkr

This comment has been minimized.

Member

parkr commented Jul 13, 2013

@maul-esel If it's not used then we don't need it, ya! But we need to keep the current configuration options absolutely the same (in terms of coderay, etc) so we'll need at least the coderay stuff,.

@zachgersh

This comment has been minimized.

Contributor

zachgersh commented Jul 13, 2013

@parkr IRC please :)

@zachgersh

This comment has been minimized.

Contributor

zachgersh commented Jul 13, 2013

Hey @mattr- and @parkr, I am having some issues actually trying to test this properly.

Also I found that we are already testing kramdown options here:

https://github.com/mojombo/jekyll/blob/master/test/test_kramdown.rb

So what I decided on was deep_merging in a new option in:

remove_span_html_tags

Then I try to run this sort of test

assert_match /<p><\/p>/, markdown.convert(%{'<span></span>'}).strip

I was hoping that the kramdown would strip the <span> and then return just empty <p> tags. It doesn't do that and I am pretty sure the options get through but something is a bit strange.

If one of you is around on IRC tonight I think I could wrap this up fairly quickly but I need some help.

@mattr-

This comment has been minimized.

Member

mattr- commented Jul 14, 2013

Sorry, I missed this. I'm not usually around IRC until 9pm Central Time or so, which is usually sometime early morning for all y'all. 😀

@mattr-

This comment has been minimized.

Member

mattr- commented Jul 14, 2013

I 👍 this for the code, but I 😡 at @zachgersh's choice of options to test. the remove_span_html_tags option only works with a different Kramdown converter than the HTML converter. Win some, lose some. 😄

@zachgersh

This comment has been minimized.

Contributor

zachgersh commented Jul 14, 2013

Looking at this, there really aren't many more options to test:

http://kramdown.rubyforge.org/converter/html.html

Our original test was already pretty good :)

I am about to commit a few slight changes to this.

parkr added a commit that referenced this pull request Jul 14, 2013

Merge pull request #1201 from zachgersh/all_kramdown
Supporting more kramdown options.

@parkr parkr merged commit 4ce89ac into jekyll:master Jul 14, 2013

1 check passed

default The Travis CI build passed
Details
@@ -13,7 +13,7 @@ def initialize(config)
def convert(content)
# Check for use of coderay
kramdown_configs = if @config['kramdown']['use_coderay']
if @config['kramdown']['use_coderay']
base_kramdown_configs.merge({

This comment has been minimized.

@parkr

parkr Jul 14, 2013

Member

Whoops, forgot a call to the method

parkr added a commit that referenced this pull request Jul 14, 2013

parkr added a commit that referenced this pull request Jul 14, 2013

parkr added a commit that referenced this pull request Jul 14, 2013

@mattr-

This comment has been minimized.

Member

mattr- commented Jul 15, 2013

Gasp! I'm not 😡 at @zachgersh's choice of option, but rather at Kramdown's weird implementation of stripping HTML tags from documents and the fact that the option doesn't work with the HTML converter it provides. Sorry @zachgersh 😭

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017

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