Skip to content

Switch from pulldown-cmark to comrak #981

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

Merged
merged 6 commits into from
Aug 26, 2017
Merged

Switch from pulldown-cmark to comrak #981

merged 6 commits into from
Aug 26, 2017

Conversation

anthonynguyen
Copy link
Contributor

Fixes #978

2017-08-19-235915_693x247_scrot

This only enables the task list feature. There are other GFM features that aren't enabled.

@anthonynguyen
Copy link
Contributor Author

Seems to fix #966 as well

2017-08-20-001118_680x672_scrot

@vignesh-sankaran
Copy link
Contributor

@anthonynguyen Could you run cargo fmt against your code and reupload please? :)

@kureuil
Copy link
Contributor

kureuil commented Aug 20, 2017

I'm not sure about this... I initially chose pulldown-cmark because it was chosen by the rustdoc team to render crates documentation and I wanted to re-use as much work as possible from the rustdoc migration. Also, rust-lang/rfcs#1990 seems pretty important to me ?

@kivikakk as the maintainer of comrak, do you have any thoughts on this ?
This might not be as big a problem as I think.

@kivikakk
Copy link
Contributor

I haven't got any specific thoughts; comrak was created (a) to keep something as close to the reference C implementation as possible, so that changes in the spec could be adapted to quickly without effort, and (b) to have full support for GFM, since I maintain that for GitHub in our fork of the C version. (pulldown-cmark's parsing strategy is very different so the changes can't be taken 1:1.)

There was a period of inactivity, but pulldown-cmark seems to be getting regularly updated again recently. It's still failing 54 out of 624 examples in the latest CommonMark spec. I'll be keeping comrak up to date for the forseeable future as other projects come to depend on it (e.g. docs.rs, among others), but I see a lot of value in keeping crates.io at parity with rustdoc.

@carols10cents
Copy link
Member

Since people write their readmes with GFM tables, checkmarks, etc, I think we should try to have parity with GFM for now. Maybe someday there will actually be one markdown standard.................. lol.

Using the same renderer as docs.rs sounds great to me! It's still pretty early for the new rustdoc, so when that becomes production-ready, we can re-evaluate at that point. Right now, though, READMEs aren't included in rustdoc by default. RFC 1990 should make it easier to include README text in docs, so we may have some re-evaluating to do once that's implemented as well.

Testing this out now!

@carols10cents
Copy link
Member

Yeah I think we want tables enabled: #983

@anthonynguyen
Copy link
Contributor Author

Should strikethrough be enabled as well @carols10cents?

@kivikakk
Copy link
Contributor

Not presuming, but if we want parity with how GitHub would render a README.md, the extensions to enable are all of them, per github-markup:

https://github.com/github/markup/blob/793148bb6a577cbd9fd25650d01593911ff77a6d/lib/github/markup/markdown.rb#L8

@anthonynguyen
Copy link
Contributor Author

Seems reasonable

@anthonynguyen
Copy link
Contributor Author

The tagfilter extension is at odds with the Ammonia cleaning that's done, so I'm not sure which one we'd rather have

@carols10cents
Copy link
Member

What's at odds about it?

@anthonynguyen
Copy link
Contributor Author

anthonynguyen commented Aug 22, 2017

I guess what I mean is there's some overlap in their functionality. tagfilter will mangle a select few tags, so it acts as a blacklist, whereas Ammonia acts as a whitelist.

@carols10cents
Copy link
Member

Mmmm I think I'd prefer the list of allowed tags that Ammonia uses then.

@steveklabnik
Copy link
Member

To chime in slightly, pulldown-cmark was chosen because comrak didn't exist at the time we made the decision; pulldown-cmark was the only commonmark equivalent implementation that existed.

I've spoken to pulldown-cmark's maintainer about GFM equivalence, it's someting we'd all like, but it's not clear who is going to have the time and when to update it.

@kivikakk
Copy link
Contributor

@anthonynguyen @carols10cents Just to give some background, tagfilter isn't intended as a comprehensive sanitizer and you should definitely use something on top of it even if it was used. The list of tags is specifically those that cause a HTML parser to switch parsing modes (the GFM spec touches on this).

@carols10cents
Copy link
Member

I just force pushed, sorry about that-- I took out the commit turning the tag filtering off, it sounds like to try and be as similar to github as possible, we should have the tag filtering on even though we're ALSO using ammonia to further sanitize.

Tested this out and the issues mentioned look all fixed! 🎉

bors: r+

bors-voyager bot added a commit that referenced this pull request Aug 26, 2017
981: Switch from `pulldown-cmark` to `comrak` r=carols10cents

Fixes #978 

![2017-08-19-235915_693x247_scrot](https://user-images.githubusercontent.com/1484987/29491961-788634f6-853a-11e7-89b3-a11606c0af82.png)

This only enables the task list feature. There are [other GFM features] that aren't enabled.

[other GFM features]: https://docs.rs/comrak/0.1.7/comrak/struct.ComrakOptions.html
@bors-voyager
Copy link
Contributor

bors-voyager bot commented Aug 26, 2017

Build failed

@carols10cents
Copy link
Member

oops looks like i broke something, fixing

I'd rather start debugging from the diff of the expected vs actual text,
rather than "expected true got false".
@carols10cents
Copy link
Member

bors: r+

bors-voyager bot added a commit that referenced this pull request Aug 26, 2017
981: Switch from `pulldown-cmark` to `comrak` r=carols10cents

Fixes #978 

![2017-08-19-235915_693x247_scrot](https://user-images.githubusercontent.com/1484987/29491961-788634f6-853a-11e7-89b3-a11606c0af82.png)

This only enables the task list feature. There are [other GFM features] that aren't enabled.

[other GFM features]: https://docs.rs/comrak/0.1.7/comrak/struct.ComrakOptions.html
@bors-voyager
Copy link
Contributor

bors-voyager bot commented Aug 26, 2017

Build succeeded

@bors-voyager bors-voyager bot merged commit 7eedf1b into rust-lang:master Aug 26, 2017
bors-voyager bot added a commit that referenced this pull request Aug 26, 2017
1010: Readme rendering - only rerender older than a date, only rerender for a crate r=carols10cents

Only rerender in a `Box<T>`, only rerender in Firefox :)

These changes add some arguments to the render-readmes script to make it easier to restart a failed run only with those readmes that have not been rendered since a particular date, and to rerender one particular crate's readmes.

By default, the render-readmes script still rerenders all readmes of all versions. But if the script is killed partway through, it doesn't need to be completely restarted-- you can grab the time that the script printed out that it started, then rerun with `--older-than thattime`, and it'll only try and render the ones it hasn't gotten to yet!

Another thing I've wanted is the ability to render one particular crate's readmes, so that I don't have to scroll through all the logs to see why a particular crate failed. So I added the `--crate` argument too.

I'd like to get this into the same deploy as #981 since that requires rerendering everything again.

@kureuil, could you look over this and see if it looks like I might be breaking anything?
@anthonynguyen
Copy link
Contributor Author

Cool, thanks @carols10cents!

@anthonynguyen anthonynguyen deleted the fix/md-checkbox branch August 26, 2017 21:42
@carols10cents
Copy link
Member

Thank YOU! ❤️

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.

7 participants