Skip to content
This repository has been archived by the owner on Jan 3, 2018. It is now read-only.

Work around a nested list formatting bug in the default markdown parser #98

Merged
merged 2 commits into from
May 21, 2013

Conversation

jonc125
Copy link
Contributor

@jonc125 jonc125 commented May 14, 2013

This isolates the single fix from pull request #97.
See bhollis/maruku#64 for original bug.
Using the rdiscount parser instead worked for the Oxford DTC boot camp.

@wking
Copy link
Contributor

wking commented May 14, 2013

On Tue, May 14, 2013 at 09:33:20AM -0700, Jonathan Cooper wrote:

  • Work around a nested list formatting bug in the default GitHub markdown parser.

To get started on the completely wrong foot, it's good to have a
one-line commit summary, followed by a blank line, followed by the
commit body. Otherwise git log --oneline and friends get confused.

Back to the main topic.

Since the Maruku issue is closed and stagnant, I'm in favor of this
PR, but I'm not sure what the best way of testing it is. Possibly a
recursive diff of local Jekyll runs using both Markdown libraries?

@ahmadia
Copy link
Contributor

ahmadia commented May 14, 2013

Reading around online, it seems that if we're switching from Jekyll's default markdown parser (maruku), we might as well upgrade ourselves all the way to redcarpet, which is the basis for GitHuB Flavored Markdown, including code fence syntax highlighting. I don't think we'll get auto-linking to github issues/commits/etc, but it's the parser that we interact with daily on GitHub.

It looks like redcarpet (version 2.2.2), is available from the Jekyll used by GitHub Pages, and it matches best with everyone's Markdown expectations.

@jonc125
Copy link
Contributor Author

jonc125 commented May 14, 2013

@wking I'm guessing running git commit --amend to fix the commit message isn't going to play happily with the pull request open?

Happy to add another commit switching to redcarpet if that is preferred.

@ahmadia
Copy link
Contributor

ahmadia commented May 14, 2013

@jonc125 - it's no problem to fix your branch however you want and force push a new branch with the same name. GitHub will do the right thing.

My vote is for redcarpet but perhaps we should ping @jiffyclub, since I believe he set these pages up originally.

I like @wking's suggestion to diff the html outputs, is there a script lying around somewhere that generates the html?

@wking
Copy link
Contributor

wking commented May 14, 2013

On Tue, May 14, 2013 at 11:29:37AM -0700, ahmadia wrote:

I like @wking's suggestion to diff the html outputs, is there a
script lying around somewhere that generates the html?

Nope! Hooray commodity site compilers :D. Just run jekyll --pygments --no-lsi --safe (as pointed out in the wiki 1 ;).

@ahmadia
Copy link
Contributor

ahmadia commented May 18, 2013

Here's a summary of what I've found so far:

First off, the new way to build with recent versions of jekyll is just:

jekyl build

This will populate the _site directory with content from _posts.

Second, as far as I can tell, there are many minor differences between the way the parsers handle things:

https://gist.github.com/ahmadia/5605072

Notably:

  • maruku automatically tags headers with ids, neither rdiscount or redcarpet do this.
  • Simple Quote-Handling
    • maruku just leaves " "
    • rdiscount/redcarpet transform to &quot
  • Smart Quote-Handling
    • maruku has it, rdiscount leaves as ", redcarpet transforms to &quot
  • Fenced Code Blocks
    • only implemented by redcarpet, coming in rdiscount eventually.
  • maruku complains when you hand it poorly formatted Markdown, which is nice
  • maruku is no longer maintained, and doesn't correctly implement Markdown

Ignorably:

maruku doesn't preserve line breaks in the Markdown source, uglifying the HTML somewhat
maruku doesn't correct '' into "" in html tags.

Conclusion

I am going to leave this pull request open for a bit more (it needs to be rebased before merge). The bugs in maruku are prominent enough to prompt a change in parsers. I recommend that we switch to redcarpet for better integration with the GitHub experience, I don't think rdiscount would be a bad choice, but it doesn't have any strong advantages over redcarpet that I'm aware of.

@jiffyclub
Copy link
Contributor

I think switching to redcarpet would probably be fine.

@jonc125
Copy link
Contributor Author

jonc125 commented May 21, 2013

Hmm, I seem to have rather complicated the history here! I was trying to do Aron's rebase suggestion on a transatlantic flight. Shall I try to clean it up, or will it merge OK as-is?

@ahmadia
Copy link
Contributor

ahmadia commented May 21, 2013

It definitely needs to be cleaned up before we merge. Can you do something along the lines of:

 git remote add swc https://github.com/swcarpentry/boot-camps
 git fetch swc gh-pages:replace-maruku
 git checkout replace-maruku
 # modify _config.yaml  (you would cherry-pick commits here if it wasn't a one-line change)
 git commit 
 # assuming your github/jonc repository is aliased to origin
 git push -f origin replace-maruku:nested-lists-fix

This will give you a clean commit descending from the latest commit in master. I haven't heard any howls we'll merge this in as soon as it hits.

jonc125 added 2 commits May 21, 2013 14:01
The default maruku parser has an issue with nested lists;
see bhollis/maruku#64 for details.
Using the rdiscount parser instead worked for the Oxford DTC boot camp.
@jonc125
Copy link
Contributor Author

jonc125 commented May 21, 2013

There, that looks better.

@ahmadia
Copy link
Contributor

ahmadia commented May 21, 2013

Substantially. Merged

ahmadia added a commit that referenced this pull request May 21, 2013
Work around a nested list formatting bug in the default markdown parser
@ahmadia ahmadia merged commit a3cd336 into swcarpentry:gh-pages May 21, 2013
@ahmadia
Copy link
Contributor

ahmadia commented May 21, 2013

Whew. Thanks for sticking through this one @jonc125. I hope you'll stick around a bit to help out with improving some other aspects of the content :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants