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

Added strikethrough syntax to GitHub Flavored Markdown parser #307

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@parkr
Contributor

parkr commented Feb 8, 2016

Duplicates #184 by @diegobg. Fixes #304.

@gettalong We'd like to work with you to bring this GFM parser up to speed with the current offerings on GitHub. Expect some PR's to that effect in the coming weeks. Thank you for kramdown!

@parkr

This comment has been minimized.

Contributor

parkr commented Feb 8, 2016

@gettalong Do you require a certain Ubuntu version for Travis?

E: Unable to locate package texlive-latex-base
E: Unable to locate package texlive-latex-recommended
E: Unable to locate package texlive-latex-extra
@parkr

This comment has been minimized.

Contributor

parkr commented Feb 8, 2016

@gettalong Tests are passing on all but Ruby 1.8.7. May I ask why you still support 1.8.7? It's been EOL'd a really long time ago and it's generally A Bad Idea to use it now after a number of years in disrepair. The 1.8.7 error:

-<p>I <del>don\342\200\231t even</del>~ have an extra tilde.</p>
+<p>I <del>don&#8217;t even</del>~ have an extra tilde.</p>
@parkr

This comment has been minimized.

Contributor

parkr commented Feb 8, 2016

/cc @benbalter, for your notes. I have this working.

@parkr

This comment has been minimized.

Contributor

parkr commented Feb 10, 2016

@gettalong Do you have time to review this? If not, would you consider adding me as a collaborator?

@geraldb

This comment has been minimized.

geraldb commented Feb 11, 2016

@parkr FYI: I have no inside knowledge (but I have confirmed Thomas Leitner for a talk on Tue Feb/23 in Vienna) - as far as I know Thomas might be on skiing vacations - thus, you will need a little patience until you will get a response.

@parkr

This comment has been minimized.

Contributor

parkr commented Feb 11, 2016

@geraldb Good to know! Thanks for the update.

@gettalong

This comment has been minimized.

Owner

gettalong commented Feb 14, 2016

@parkr As @geraldb mentioned I was on vacation the last fortnight - I will try to look at the issues/pull requests the next few days.

@@ -19,6 +19,7 @@ changes:
* Support for fenced code blocks using three or more backticks has been added.
* Hard line breaks in paragraphs are enforced by default (see option 'hard_wrap').
* ATX headers need a whitespace character after the hash signs.
* Strikethroughs can be created using two or more tildes surrounding a piece of text

This comment has been minimized.

@gettalong

gettalong Feb 16, 2016

Owner

It seems that only two tildes are only ever used, not "two or more".

@@ -60,6 +64,30 @@ def add_hard_line_breaks(element)
FENCED_CODEBLOCK_MATCH = /^(([~`]){3,})\s*?(\w[\w-]*)?\s*?\n(.*?)^\1\2*\s*?\n/m
define_parser(:codeblock_fenced_gfm, /^[~`]{3,}/, nil, 'parse_codeblock_fenced')
STRIKETHROUGH_DELIM = /~~/
STRIKETHROUGH_MATCH = /#{STRIKETHROUGH_DELIM}[^\s~](.*)[^\s~]#{STRIKETHROUGH_DELIM}/m
define_parser(:strikethrough_gfm, STRIKETHROUGH_MATCH, '~~')

This comment has been minimized.

@gettalong

gettalong Feb 16, 2016

Owner

Why are you potentially matching more than needed? It should be sufficient to use STRIKETHROUGH_DELIM here instead of STRIKETHROUGH_MATCH.

@gettalong

This comment has been minimized.

Owner

gettalong commented Feb 16, 2016

@parkr You can ignore the 1.8.7 failure for GFM, this is restriction is only for the main kramdown parser. As why it is still supported: There is currently no reason to not support it in 1.x and there are apparently folks still using 1.8.x.

@gettalong gettalong self-assigned this Feb 16, 2016

@parkr

This comment has been minimized.

Contributor

parkr commented Feb 16, 2016

@gettalong Updated. I also skipped all GFM tests for 1.8.7.

@gettalong

This comment has been minimized.

Owner

gettalong commented Feb 17, 2016

Thanks! One more thing: Please combine all GFM strikethrough related commits (ie. all except the one concerning TravisCI) into one commit.

@gettalong

This comment has been minimized.

Owner

gettalong commented Feb 17, 2016

@parkr After the STRIKETHROUGH_DELIM change the test is failing because whitespace after the initial/before the final delimiter is not correctly handled.

Two options: Either restore the original behavior (which is potentially much slower) or look at the emphasis parsing implementation to see how to handle this situation.

@parkr

This comment has been minimized.

Contributor

parkr commented Feb 18, 2016

look at the emphasis parsing implementation to see how to handle this situation.

Are you suggesting that I copy the implementation/logic for the emphasis parser?

@gettalong

This comment has been minimized.

Owner

gettalong commented Feb 18, 2016

You probably don't need all the logic from there but some logic needs to be included to make sure

  • that the final ~~ is really found and not just the end of the string
  • that there is no whitespace after the initial and before the final delimiter
@gettalong

This comment has been minimized.

Owner

gettalong commented Feb 18, 2016

@parkr The mathjax-node related errors are now fixed thanks to @tmthrgd. I have also already incorporated your Travis fix into the repo.

@parkr

This comment has been minimized.

Contributor

parkr commented Feb 22, 2016

@gettalong Rebased on latest master.

that the final ~~ is really found and not just the end of the string

Added test case. It passes.

that there is no whitespace after the initial and before the final delimiter

Added test case. It passes.

Anything else? I don't know a better way to accomplish this that's more efficient. Feel free to take our two commits and improve upon them, but I believe this works just fine. 😄

@parkr

This comment has been minimized.

Contributor

parkr commented Feb 24, 2016

@gettalong Let me know if there is anything else needed here. I would love to see this feature make it into the next version of kramdown. Thanks!

@gettalong

This comment has been minimized.

Owner

gettalong commented Feb 26, 2016

There is still at least one edge case that is not handled correctly with the current code - I will look at it.

@gettalong

This comment has been minimized.

Owner

gettalong commented Feb 26, 2016

Before I do anything further could someone please tell me the GFM specification for strikethrough?

There are two possibilities:

  1. Test any input in an input field like this one in which I'm writing and try to do whatever we can find out.
  2. Do the correct thing, even for edge cases.

The reason why I ask is the following example:

This ~~is a complex strike through *test ~~with nesting~~ involved*.

This ~~is a complex strike through *test with nesting involved* .

It currently gives the following result when evaluated in this input field:

<p>This <del>is a complex strike through *test ~~with nesting</del> involved*.</p>

As you can see this is not good...

So, what should be done? Support GFM like it is now or support something working in a useful way?

@parkr

This comment has been minimized.

Contributor

parkr commented Feb 27, 2016

Thanks for looking into that.

So, what should be done? Support GFM like it is now or support something working in a useful way?

This is a question for @benbalter. My thoughts are to support it as it is rather than how it ought to be. If it makes our code more complex to support it as it is, then I am fine with this breakage. 👍

@gettalong

This comment has been minimized.

Owner

gettalong commented Feb 27, 2016

Since GFM is not my main concern it would be okay if it is broken by design (in contrast to the kramdown parser). However, note that if this is how the GFM parser should work it will be announced as such in kramdown's documentation to make this clear and tests for the broken design will also be added.

I also had to make this choice for kramdown initially: Support the broken Markdown.pl way or break 100% compatibility to do something useful and, especially, predictive. The latter was my choice for kramdown (though there are some things I would change).

@benbalter

This comment has been minimized.

benbalter commented Feb 29, 2016

Until we move to something like CommonMark, there's no such thing as a GFM spec, AFAIK, so there's not necessarily a "right" way to do things, in my mind.

Since GFM is not my main concern it would be okay if it is broken by design (in contrast to the kramdown parser).

@gettalong based on your comment above, it sounds like you see the GFM parser primarily as something to give parity to GitHub Markdown. If that's the case, then my vote would be for mirroring GitHub's implementation, even if it's silly.

My reasoning is that if I push a file to GitHub.com (which uses GFM), and then they try to render it using Kramdown (which advertises GFM support) and the two differ, my first reaction would be to file a bug, because the software violated my expectations (regardless of if the behavior was right or not).

If you see GFM support as a first-class, but alternative parser for those that prefer GFM (but may not use it with regard to GitHub's implementation), then the more philosophically pure implementation makes sense. Otherwise, I'd err on the side of practicality and user expectations, unless that adds unnecessary complexity to the code.

FWIW, I wrote up a quick test to compare common markdown fixtures rendered with Kramdown and with the GitHub Markdown API, and these seems to be the only major point of differentiation, once e.g., whitespace is normalized.

(Also to note, I did some spelunking through the github-markdown implementation and history, but couldn't find any guidance one way or the other).

@geraldb

This comment has been minimized.

geraldb commented Feb 29, 2016

If I may add my two cents @gettalong @benbalter @parkr

Why not split-off the kramdown GFM Parser class into a new gem and its own project e.g. kramdown-gfm or something. Why? First the code is actually so far minor e.g. about 100+ lines of ruby - see Kramdown::Parser::GFM.

Thomas Leitner - as I talked to him - if I dare to say has no interest really in supporting the GitHub-Flavored (GFM) markdown - also as said before - Thomas is not using Jekyll either ;-). His main concern is the "core" kramdown syntax (without any extensions). If you use your own (split-off) addon and gem than you can change the code as you like and that's in the best interest and you will get the support and approval of Thomas Leitner as far as I understand (from our in-person conversations). And it makes sense. As a real-world case study example there's already the kramdon-rfc2629 parser by Carsten Bormann.

Why should Thomas in his spare time support the GitHub-Flavored (GFM) addon / syntax? I'd say that is not really optimal for getting updates and changes included in Jekyll and GitHub Pages. Using your own kramdown-gfm addon - you can update and push changes whenver you like and Thomas and others can still contribute (it's open source on GitHub ;-) And again the code involved is minimal - maybe two or three hundred lines of Ruby - if you add the new strikethrough and autolink extensions? Really not much more involved than a Jekyll plugin. Than it's easy to add strikethrough and some other minor missing extras without worring about the need for a spec - it's your code and that's good enough - the code is the spec ;-) Keep it simple. Onwards. Cheers.

@gettalong

This comment has been minimized.

Owner

gettalong commented Feb 29, 2016

@benbalter I don't intend the GFM parser to be a standalone, separate Markdown parser that is used outside the context of Github and therefore "first-class". GFM makes, in my opinion, only sense when used in the context of Github, eg. Github Pages.

So if there is no GFM spec to work with or test against, I'm fine with including the changes in this way.

@geraldb Splitting off the GFM parser would be a possibility but is currently not really necessary as contributing to the GFM parser inside the kramdown project is possible.

@gettalong gettalong closed this in ac71ef7 Feb 29, 2016

@gettalong

This comment has been minimized.

Owner

gettalong commented Feb 29, 2016

The parser needed some more work to mimic Github in respect to my "broken" example. I also removed some unneeded code in regards to the escaping mechanism and fixed the tests.

Thanks for the input and discussion!

@parkr parkr deleted the parkr:strikethrough-gfm branch Feb 29, 2016

@parkr

This comment has been minimized.

Contributor

parkr commented Feb 29, 2016

@gettalong Thank you! And thank you for preserving our commits in your merge. Cheers! 🍻

@captn3m0 captn3m0 referenced this pull request Mar 21, 2017

Closed

Migrate to Jekyll v3 #89

0 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment