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

Modernize Kramdown for Markdown converter. #4109

Merged
merged 3 commits into from Dec 4, 2015

Conversation

Projects
None yet
3 participants
@envygeeks
Contributor

envygeeks commented Nov 4, 2015

No description provided.

@envygeeks envygeeks added this to the 3.1 milestone Nov 4, 2015

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Nov 4, 2015

Member

@envygeeks when you comment on the diff for a PR, it's hard to see what you're commenting on.

Member

parkr commented Nov 4, 2015

@envygeeks when you comment on the diff for a PR, it's hard to see what you're commenting on.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Nov 4, 2015

Contributor

@parkr what do you mean? That's standard behavior that is always done.

Contributor

envygeeks commented Nov 4, 2015

@parkr what do you mean? That's standard behavior that is always done.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Nov 4, 2015

Member

@parkr what do you mean? That's standard behavior that is always done.

I have never done this before, I always comment on the PR diff. The comments on a PR diff are hidden once you update the PR, whereas comments on a commit are never hidden. I can see you have two kinds of comments: (1) a pre-emptive "i'm going to fix this, don't worry" comment, and (2) a "for your information" comment. I think comment 1 is better on the PR diff, whereas 2 works great on the commit.

Member

parkr commented Nov 4, 2015

@parkr what do you mean? That's standard behavior that is always done.

I have never done this before, I always comment on the PR diff. The comments on a PR diff are hidden once you update the PR, whereas comments on a commit are never hidden. I can see you have two kinds of comments: (1) a pre-emptive "i'm going to fix this, don't worry" comment, and (2) a "for your information" comment. I think comment 1 is better on the PR diff, whereas 2 works great on the commit.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Nov 4, 2015

Contributor

The assumption here is that it's not a mitigation before another commit. The problem is most of those changes should be obvious but after the fact I mitigated potential question before it happened because there is no plan to update this pull for a few days because there are more important things in the backlog than this, this was just fullfilling a comment on the problems with the processor and mitigating once the pull was made until the PR can be updated with the changes.

Both of those fall into category 1 in this circumstance.

Contributor

envygeeks commented Nov 4, 2015

The assumption here is that it's not a mitigation before another commit. The problem is most of those changes should be obvious but after the fact I mitigated potential question before it happened because there is no plan to update this pull for a few days because there are more important things in the backlog than this, this was just fullfilling a comment on the problems with the processor and mitigating once the pull was made until the PR can be updated with the changes.

Both of those fall into category 1 in this circumstance.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Nov 4, 2015

Contributor

It's natural for me to mitigate questions if I do not plan work on something before those questions come, that way I don't have to backtrack and answer those questions I foresaw. It's natural behavior for me.

Contributor

envygeeks commented Nov 4, 2015

It's natural for me to mitigate questions if I do not plan work on something before those questions come, that way I don't have to backtrack and answer those questions I foresaw. It's natural behavior for me.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Nov 5, 2015

Member

Ok. Let me know when this is ready!

Member

parkr commented Nov 5, 2015

Ok. Let me know when this is ready!

@parkr parkr added the needs-work label Nov 5, 2015

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Nov 18, 2015

Contributor

/cc @jekyll/core

Contributor

envygeeks commented Nov 18, 2015

/cc @jekyll/core

Show outdated Hide outdated lib/jekyll/converters/markdown/kramdown_parser.rb
module Jekyll
module Converters
class Markdown
class KramdownParser
CoderayDefaults = {

This comment has been minimized.

@parkr

parkr Nov 18, 2015

Member

We usually do SCREAMING_SNAKE_CASE for constants.

@parkr

parkr Nov 18, 2015

Member

We usually do SCREAMING_SNAKE_CASE for constants.

This comment has been minimized.

@parkr

parkr Nov 18, 2015

Member

^

@parkr
Show outdated Hide outdated lib/jekyll/converters/markdown/kramdown_parser.rb
end
private
def load_kramdown

This comment has been minimized.

@parkr

parkr Nov 18, 2015

Member

this can be replaced by a call to Jekyll::External.gracefully_require 'kramdown' which has an error message and such built into it

@parkr

parkr Nov 18, 2015

Member

this can be replaced by a call to Jekyll::External.gracefully_require 'kramdown' which has an error message and such built into it

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Nov 18, 2015

Member

@envygeeks I'm confused by all the stuff you're doing with configuration in this converter. Maybe you can explain that to me?

Member

parkr commented Nov 18, 2015

@envygeeks I'm confused by all the stuff you're doing with configuration in this converter. Maybe you can explain that to me?

Show outdated Hide outdated lib/jekyll/converters/markdown/kramdown_parser.rb
hash.inject({}) do |hash_, (key, val)|
cleaned_key = key.gsub(/\Acoderay_/, "")
if hash.has_key?(key)
Jekyll.logger.warn "You are using an old CodeRay key: '#{key}'." \

This comment has been minimized.

@parkr

parkr Nov 18, 2015

Member

Is this considered a deprecation?

@parkr

parkr Nov 18, 2015

Member

Is this considered a deprecation?

This comment has been minimized.

@envygeeks

envygeeks Nov 18, 2015

Contributor

Yessum.

@envygeeks

envygeeks Nov 18, 2015

Contributor

Yessum.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Nov 18, 2015

Contributor

It normalizes and modernizes everything that was legacy and supported in old Jekyll until 4.0 were all that will be stripped out and only "syntax_highlighter_opts", "syntax_highlighter" and "highlighter" will be supported.

Right now Kramdown uses syntax_highlighter_opts: http://kramdown.gettalong.org/options.html#option-syntax-highlighter-opts and syntax_highlighter

So what we do is 1. Search for "enable_coderay" and tell them NO use "syntax_highlighter" on Kramdown, enable_coderay is old.. but we still let it pass and kick on coderay for them and 2. "highlighter" which is what Jekyll allows so we say nothing and we alias it over to what Kramdown prefers.

Then after we've done that, we then go through and normalize the configuration. First we check to see if you have the modern version of Kramdown "syntax_hilighter_opts", then we search to see if you have "coderay" and if you do we merge them and then lastly since "coderay_" is old, we then go onto normalize that as non "coderay_" on "syntax_highlighter_opts" and ship a warning that "coderay_" is old, modernize your configuration.

In code:

{
"kramdown": {
  "enable_coderay": true
  "coderay_opt1": value
  "coderay": {
    "opt2": value
  }
}

becomes:

{
"kramdown": {
  "syntax_highlighter": :coderay,
  "syntax_highlighter_opts": {
    "opt1": value,
    "opt2": value
}
Contributor

envygeeks commented Nov 18, 2015

It normalizes and modernizes everything that was legacy and supported in old Jekyll until 4.0 were all that will be stripped out and only "syntax_highlighter_opts", "syntax_highlighter" and "highlighter" will be supported.

Right now Kramdown uses syntax_highlighter_opts: http://kramdown.gettalong.org/options.html#option-syntax-highlighter-opts and syntax_highlighter

So what we do is 1. Search for "enable_coderay" and tell them NO use "syntax_highlighter" on Kramdown, enable_coderay is old.. but we still let it pass and kick on coderay for them and 2. "highlighter" which is what Jekyll allows so we say nothing and we alias it over to what Kramdown prefers.

Then after we've done that, we then go through and normalize the configuration. First we check to see if you have the modern version of Kramdown "syntax_hilighter_opts", then we search to see if you have "coderay" and if you do we merge them and then lastly since "coderay_" is old, we then go onto normalize that as non "coderay_" on "syntax_highlighter_opts" and ship a warning that "coderay_" is old, modernize your configuration.

In code:

{
"kramdown": {
  "enable_coderay": true
  "coderay_opt1": value
  "coderay": {
    "opt2": value
  }
}

becomes:

{
"kramdown": {
  "syntax_highlighter": :coderay,
  "syntax_highlighter_opts": {
    "opt1": value,
    "opt2": value
}
Show outdated Hide outdated lib/jekyll/converters/markdown/kramdown_parser.rb
cleaned_key = key.gsub(/\Acoderay_/, "")
if hash.has_key?(key)
Jekyll.logger.warn "You are using an old CodeRay key: '#{key}'." \
"It is being normalized to #{cleaned_key}."

This comment has been minimized.

@parkr

parkr Nov 18, 2015

Member

what is this doing? is it supposed to be appended to the above string?

@parkr

parkr Nov 18, 2015

Member

what is this doing? is it supposed to be appended to the above string?

This comment has been minimized.

@envygeeks

envygeeks Nov 18, 2015

Contributor

Yes, that's standard Ruby behavior for split line strings that do not break.

@envygeeks

envygeeks Nov 18, 2015

Contributor

Yes, that's standard Ruby behavior for split line strings that do not break.

This comment has been minimized.

@parkr

parkr Nov 18, 2015

Member

ok, i didn't know you could close the string, it seems like it's

Jekyll.logger.warn "You are ..." "It is being"

which would be a syntax error

@parkr

parkr Nov 18, 2015

Member

ok, i didn't know you could close the string, it seems like it's

Jekyll.logger.warn "You are ..." "It is being"

which would be a syntax error

This comment has been minimized.

@envygeeks

envygeeks Dec 4, 2015

Contributor

I shortened the message to make it clearer to people who don't know about that trick.

@envygeeks

envygeeks Dec 4, 2015

Contributor

I shortened the message to make it clearer to people who don't know about that trick.

Show outdated Hide outdated lib/jekyll/converters/markdown/kramdown_parser.rb
ogconfg = @config["syntax_highlighter_opts"]
coderay = Jekyll::Utils.deep_merge_hashes(CoderayDefaults, @config["coderay"]) # XXX: Legacy
@config["syntax_highlighter_opts"] = strip_coderay_prefix(Jekyll::Utils.deep_merge_hashes(
ogconfg, coderay

This comment has been minimized.

@parkr

parkr Nov 18, 2015

Member

this could just be @config["syntax_highlighter_opts"], coderay

@parkr

parkr Nov 18, 2015

Member

this could just be @config["syntax_highlighter_opts"], coderay

This comment has been minimized.

@envygeeks

envygeeks Nov 18, 2015

Contributor

Clarify because what you said doesn't make sense to me, if it's about the strip stuff, then no it can't because we are intentionally non-destructive.

@envygeeks

envygeeks Nov 18, 2015

Contributor

Clarify because what you said doesn't make sense to me, if it's about the strip stuff, then no it can't because we are intentionally non-destructive.

This comment has been minimized.

@envygeeks

envygeeks Nov 18, 2015

Contributor

Oh I see what you mean, I do not agree, unlike most programmer I like to stick close to 80c and I do break it in some spots but that's because it would kill readability.

@envygeeks

envygeeks Nov 18, 2015

Contributor

Oh I see what you mean, I do not agree, unlike most programmer I like to stick close to 80c and I do break it in some spots but that's because it would kill readability.

This comment has been minimized.

@parkr

parkr Nov 18, 2015

Member

Jekyll::Utils.deep_merge_hashes creates a dup of the base hash so it's not destructive to the original hash

80c

we haven't done 80 char widths in a while – i tried to push for them but it became too absurd to ask and no one ever followed it when contributing. We nest things a lot, as it turns out, and it hurt readability more to put things on new lines than to be 5 characters over

@parkr

parkr Nov 18, 2015

Member

Jekyll::Utils.deep_merge_hashes creates a dup of the base hash so it's not destructive to the original hash

80c

we haven't done 80 char widths in a while – i tried to push for them but it became too absurd to ask and no one ever followed it when contributing. We nest things a lot, as it turns out, and it hurt readability more to put things on new lines than to be 5 characters over

This comment has been minimized.

@envygeeks

envygeeks Nov 18, 2015

Contributor

I'm not ready to let go so I'll stick with 80c 😜 but I can probably rework that to be cleaner anyways since I always thought it was rather sloppy, and technically I don't need to deep merge so removing that will instantly appease both of us.

@envygeeks

envygeeks Nov 18, 2015

Contributor

I'm not ready to let go so I'll stick with 80c 😜 but I can probably rework that to be cleaner anyways since I always thought it was rather sloppy, and technically I don't need to deep merge so removing that will instantly appease both of us.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Nov 18, 2015

Member

3 thoughts above, then 👍

Member

parkr commented Nov 18, 2015

3 thoughts above, then 👍

Show outdated Hide outdated lib/jekyll/converters/markdown/kramdown_parser.rb
private
def strip_coderay_prefix(hash)
hash.inject({}) do |hash_, (key, val)|
cleaned_key = key.gsub(/\Acoderay_/, "")

This comment has been minimized.

@parkr

parkr Dec 1, 2015

Member

Could we do a sub call here? To make things faster?

@parkr

parkr Dec 1, 2015

Member

Could we do a sub call here? To make things faster?

This comment has been minimized.

@envygeeks

envygeeks Dec 4, 2015

Contributor

I've moved this to using the fastest of all, Hash#each_with_object

@envygeeks

envygeeks Dec 4, 2015

Contributor

I've moved this to using the fastest of all, Hash#each_with_object

Show outdated Hide outdated lib/jekyll/converters/markdown/kramdown_parser.rb
def highlighter
@highlighter ||= begin
if out = @config["syntax_highlighter"] then out
elsif @config.has_key?("enable_coderay") && @config["enable_coderay"]

This comment has been minimized.

@parkr

parkr Dec 1, 2015

Member

I believe has_key? has been deprecated in favour of key?. Would you mind updating here and below?

@parkr

parkr Dec 1, 2015

Member

I believe has_key? has been deprecated in favour of key?. Would you mind updating here and below?

This comment has been minimized.

@envygeeks

envygeeks Dec 4, 2015

Contributor

Yes, I always forget about that, it's been moved to Hash#key? good catch!

@envygeeks

envygeeks Dec 4, 2015

Contributor

Yes, I always forget about that, it's been moved to Hash#key? good catch!

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Dec 1, 2015

Member

🎉

Member

parkr commented Dec 1, 2015

🎉

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Dec 3, 2015

Member

@envygeeks Know what the failure on Ruby 2.2 is about?

Member

parkr commented Dec 3, 2015

@envygeeks Know what the failure on Ruby 2.2 is about?

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Dec 3, 2015

Contributor

@parkr I don't but I'll be getting back to this and updating it tomorrow anyways so I'll look at it then. Now that the new Docker stuff for EnvyGeeks is out and released I have some free time.

Contributor

envygeeks commented Dec 3, 2015

@parkr I don't but I'll be getting back to this and updating it tomorrow anyways so I'll look at it then. Now that the new Docker stuff for EnvyGeeks is out and released I have some free time.

@envygeeks envygeeks self-assigned this Dec 4, 2015

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Dec 4, 2015

Contributor

This is ready /cc @jekyll/core

Contributor

envygeeks commented Dec 4, 2015

This is ready /cc @jekyll/core

parkr added a commit that referenced this pull request Dec 4, 2015

@parkr parkr merged commit 6cbd06e into master Dec 4, 2015

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@parkr parkr deleted the cleanup-kramdown-converter branch Dec 4, 2015

parkr added a commit that referenced this pull request Dec 4, 2015

@parkr parkr referenced this pull request Dec 10, 2015

Closed

Fix flaky test #4250

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