Skip to content
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

Add support for GitHub-flavored Markdown #67

Merged
merged 2 commits into from
Mar 19, 2018
Merged

Conversation

theacodes
Copy link
Member

This uses the cmarkgfm package to render the "gfm" variant of Markdown. It also preferentially uses cmarkgfm for CommonMark as it is CommonMark compliant (and fast).

Notes:

  • The cmarkgfm library is currently an extra dependency (readme_renderer[gfm]) mostly because I don't yet have wheels for cmarkgfm.
  • I added a ton of fixtures for gfm based on GitHub's own spec. They had to be tweaked a little because bleach changes the output very slightly (such as adding rel="nofollow").

This uses the [cmarkgfm](https://pypi.org/project/cmarkgfm/) package to render the "gfm" variant of Markdown. It also preferentially uses cmarkgfm for CommonMark as it is CommonMark compliant (and fast).

Notes:
* The cmarkgfm library is currently an *extra* dependency (``readme_renderer[gfm]``) mostly because I don't yet have wheels for cmarkgfm.
* I added a ton of fixtures for gfm based on GitHub's [own spec](https://github.github.com/gfm/). They had to be tweaked a little because bleach changes the output very slightly (such as adding `rel="nofollow"`).
@theacodes
Copy link
Member Author

Context: pypa/packaging-problems#126

@theacodes
Copy link
Member Author

/cc @di @dstufft @ewdurbin

@di
Copy link
Member

di commented Mar 19, 2018

Given how many folks will probably want GFM over CommonMark, I wonder if we should make that the default variant.

@pradyunsg
Copy link
Member

Given how many folks will probably want GFM over CommonMark, I wonder if we should make that the default variant.

+1

@theacodes
Copy link
Member Author

Given how many folks will probably want GFM over CommonMark, I wonder if we should make that the default variant.

Baby steps. Let's get this in first. Solid next steps after that are:

  1. Get wheels built for cmarkgfm.
  2. Transfer cmarkgfm over to pypa.
  3. Make cmarkgfm a non-optional dependency here, remove CommonMark (the package) dependency.

In parallel:

  1. Update warehouse to support the gfm variant of Markdown.


assert render(md_markup, variant=variant) == expected
assert render(md_markup, variant=variant).strip() == expected
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is .strip() necessary here? Can we just add/remove newlines from the fixtures as necessary?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. The reason I added this is because bleach can occasionally consume the trailing newline from things- especially when it fixes broken HTML. This is mostly just affecting on particular fixture, so I can likely just do that. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in 93589c7

@di
Copy link
Member

di commented Mar 19, 2018

Ah yeah, I meant to make that comment over at pypa/packaging-problems#126 instead. Moving...

@di
Copy link
Member

di commented Mar 19, 2018

Update warehouse to support the gfm variant of Markdown.

This should Just Work™, the only change to Warehouse necessary would be bumping the readme_renderer dependency.

@theacodes
Copy link
Member Author

@di di merged commit 8e44fa9 into pypa:master Mar 19, 2018
@di
Copy link
Member

di commented Mar 19, 2018

Thanks @jonparrott! I think we should probably hold off on a release until we do #3:

Make cmarkgfm a non-optional dependency here, remove CommonMark (the package) dependency.

@theacodes theacodes deleted the use-cmarkgfm branch March 19, 2018 22:30
@theacodes
Copy link
Member Author

That's totally fine. I'm trying to get wheels built now. If you know anyone who can help with Windows builds I'd love to chat.

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

Successfully merging this pull request may close these issues.

3 participants