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

Wrap marky-markdown #4154

Merged
merged 6 commits into from Oct 22, 2016
Merged

Wrap marky-markdown #4154

merged 6 commits into from Oct 22, 2016

Conversation

chadwhitacre
Copy link
Contributor

marky-markdown is the markdown parser that npm uses. We should use it with npm package readmes. There's a CLI that we should be able to wrap. It doesn't take stdin but we can work around that using /dev/stdin like so:

marky-markdown /dev/stdin

@aandis
Copy link
Contributor

aandis commented Oct 22, 2016

Will calling an external process on each render be performant enough? Why not use something like https://github.com/lepture/mistune?

@aandis
Copy link
Contributor

aandis commented Oct 22, 2016

Aha, github has it's own flavor of markdown and mistune and the likes may not support all those features.

@aandis
Copy link
Contributor

aandis commented Oct 22, 2016

@aandis
Copy link
Contributor

aandis commented Oct 22, 2016

grip is pretty cool but it's no different than what we're trying to do with marky-markdown.

@aandis
Copy link
Contributor

aandis commented Oct 22, 2016

do we have tests for this?

@chadwhitacre
Copy link
Contributor Author

@aandis No tests, no. This won't be called during web requests. We'll call it in an async worker when we recrawl npm (#4153).

@chadwhitacre
Copy link
Contributor Author

do we have tests for this?

No tests, no.

But we should. :-)

@chadwhitacre
Copy link
Contributor Author

@aandis For now can we add even just one test that runs some very simple markdown through marky and verifies that it comes out as HTML? We don't need to exhaustively tests marky-markdown's features.

@aandis
Copy link
Contributor

aandis commented Oct 22, 2016

🍏

@chadwhitacre
Copy link
Contributor Author

🍏

Does that mean "Ready for review?" :-)

@aandis aandis added the Review label Oct 22, 2016
@aandis
Copy link
Contributor

aandis commented Oct 22, 2016

Yep. :)

"""
echo = Popen(("echo", markdown), stdout=PIPE)
marky = Popen(("marky-markdown", "/dev/stdin"), stdin=echo.stdout, stdout=PIPE)
return Markup(marky.communicate()[0])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can't we pass markdown into the communicate call rather than the echo hack?

Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't know pipe could work with that. Nice! Done in ecdc8e0

@chadwhitacre
Copy link
Contributor Author

Sigh, looks like #4129 didn't work. :-/

@chadwhitacre
Copy link
Contributor Author

/me restarts build ...

@chadwhitacre chadwhitacre merged commit 0453620 into master Oct 22, 2016
@chadwhitacre
Copy link
Contributor Author

!m @aandis 💃

@chadwhitacre chadwhitacre deleted the marky-markdown branch October 22, 2016 15:50
@chadwhitacre chadwhitacre mentioned this pull request Oct 24, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants