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

The Markdown Pickle #5124

Closed
bep opened this issue Aug 28, 2018 · 11 comments

Comments

@bep
Copy link
Member

commented Aug 28, 2018

So, I've been waiting for Go 1.11/Go Modules to do the Blackfriday v2 upgrade (for versioning reasons). In the meantime, things have changed on the Go markdown scene.

The motivation behind the markdown fork was lack of maintenance/bug fixing in BF. This fork is now 490 commits ahead of its origin. Pretty fundamental changes. Mostly sensible from my brief look. But the performance has degraded a fair amount, mostly from one fundamental commit. I have talked to the lead maintainer about it, but we don't share the same worldview in this area. I can sympathize with him not being a big fan of outsiders having an opinion (I would probably have reacted the same), but it leaves us in a ... markdown pickle.

So, not using the new markdown fork would mean:

  • A deprecation and eventual removal of MMark support in Hugo
  • Upgrade Hugo to Blackfriday v2 and find a working solution for the maintainance problem

Thoughts?

/cc @kaushalmodi @regisphilibert @anthonyfok and gang

@onedrawingperday

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2018

What you describe sounds like a pretty hard dilemma.

But here is something that also needs to be taken into account besides the performance issue:

A casual search for MMark in the forum returns 50+ results and right here in this repo at least 28 issues (both open and closed). It looks like that if MMark is deprecated and then removed from Hugo it would disrupt the workflow of various people.

Disclaimer: I'm not using MMark myself.

@anthonyfok

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2018

Thank you for your thorough investigation!

I read through the relevant issue and could empathize with both sides of the discussion.

I am also interested in how migrating to the new markdown fork would affect Hugo's performance in the real world, but to reach that point means putting the time and effort to actual modify Hugo for the new markdown first. Maybe a bit of a gamble.

I am glad that other people like miekg and moorereason chimed-in in that issue to hopefully help move the discussion forward. And I could only wish... maybe some other kind souls could help optimize the markdown fork code and submit PRs.


Today, a new idea came into me: there might be yet another alternative, and that is to have Hugo support for all three of the following:

  • Blackfriday v2
  • Gomarkdown/markdown fork
  • Mmark v2

And, of course, let Blackfriday v2 be the default. The end user can specify the other two if they don't mind the performance degradation, as for some people, a richer Markdown syntax, e.g. that closer to CommonMark. This will bloat the Hugo executable a little bit, but not by much.

Perhaps by then people could see the performance degredation with Gomarkdown/markdown fork in real-life, and might spur the main author of the markdown into seriously optmizing the code again.

When I looked at some of the Hugo code years ago, I had a feeling that it was somewhat coupled to Blackfriday, as in when spf13 originally worked on Hugo, Blackfriday was the only supported Markdown renderer, and several functions with the name "Markdown" were actually Blackfriday specific. Though of course, the code has gone through many revisions and refactoring since then, and is already accommodating alternative renderers.

Anyhow, the integration of kjk's gomarkdown/markdown fork would be a motive for us to examine the Hugo code to find and change the code where the name "markdown" actually meant "blackfriday", and make Hugo more "renderer-agnostic".

Disclaimer: I haven't studied the relevant code in detail for a long time, and am still definitely a Go amateur, so I might be talking nonsense above.

That said, the new Blackfriday fork calling itself markdown could be utterly confusing, so when it come times to the actual integration with Hugo, whenever that maybe, may I suggest using a namespace like kjkMarkdown instead? <grin, duck, run>...

Disclaimer: Ditto. I am musing before actually looking at the code, hehe.

In summary, my suggestion is to do the following in due time:

  1. Go ahead and upgrade Hugo to Blackfriday v2.
  2. Keep Mmark v1 for the time being. IIRC, it was forked from Blackfriday v1 and the ugprade to Blackfriday v2 won't affect our current support for Mmark v1. (though I don't know if the API change in Blackfriday v2 would make our support for legacy Mmark more complicated)
  3. Add support for gomarkdown/markdown. But of course, use something other than markdown for its namespace in Hugo.
  4. Upgrade Hugo to Mmark v2.

So, to me, that would be the best of both worlds, giving the end user choices, and allowing Blackfriday v2 and the gomarkdown/markdown fork to compete with each other in real life.

Indeed, there is some cost in maintenance for yet another Markdown renderer (3 instead of 2), but hopefully the cost isn't too much.

Disclaimer: Ditto. Again, wild speculation before actually studying the relevant code...

My 2 cents. :-)

@bep

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2018

I suspect you're not really aware of the implementation and maintenance cost of what you're suggesting.

My plan is to integrate Blackfriday v2 and use their AST to eventually do all sorts of cool stuff (starting with improved ToC support etc.). People would eventually expect to have a similar experience for Mmark and "Blackfriday Classic", but there is no way we're maintaining the above++ across two different BF forks.

And gomarkdown/markdown is currently not production ready. You could claim that Blackfriday is also buggy -- true, but those bugs are at least fixable.

@moorereason

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2018

The big dill is that BFv2 is largely unmaintained, but I don't see any signs of that changing.

If the performance of gomarkdown was on par with BFv2, would you move to gomarkdown? If so, I'd give gomarkdown a little time (2wks?) to reconsider the performance-killing design decision that was made. If not, then let's go to BFv2 and move on.

None of us wants to drop support for mmark. That would be unfortunate if it comes to that.

@bep

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2018

The big dill is that BFv2 is largely unmaintained, but I don't see any signs of that changing.

Which also has its obvious upsides.

@lpar

This comment has been minimized.

Copy link

commented Sep 10, 2018

The CommonMark Go implementation has been updated for compliance with current CommonMark standards. I'd like to at least have CommonMark as an option.

@anthonyfok

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2018

@lpar, I like your suggestion too. That said, as @ bep suggested earlier in this thread, there will be a significant implementation and maintenance cost involved with supporting multiple Markdown renderers in Hugo, so please don't be disappointed if the CommonMark Go implementation at https://gitlab.com/golang-commonmark doesn't get into Hugo.

But yeah, anything could happen in the future, e.g. Blackfriday may eventually gain support for CommonMark Go too. Let's keep our fingers crossed. ;-)

@jmarca

This comment has been minimized.

Copy link

commented Oct 30, 2018

Any update or resolution of this? I hit this bug/issue in a roundabout way. I can provide my tale of woe if anyone is interested, but essentially blackfriday, pandoc, and mmark all fail me in one way or another, whereas the new mmark (or I guess, gomarkdown/markdown) work for me. I need

  • --- to be turned into em-dash, and ' be turned into (curly quote) (breaks current mmark---smartypants isn't on I guess? Or maybe I'm doing something wrong?)
  • code in html standard code blocks (breaks pandoc, see issue jgm/pandoc#3858)
  • header attributes (breaks black friday (russross/blackfriday#181)) and block level attributes (breaks pandoc)

the new mmark processor (not the old library) does everything I need, but isn't a library...

@bep bep closed this Oct 30, 2018

@jmarca

This comment has been minimized.

Copy link

commented Oct 30, 2018

@bep Can you provide a hint as to the resolution as well? I'm trying to follow along to figure out the best path forward.

@747

This comment has been minimized.

Copy link

commented Nov 8, 2018

@jmarca FYI at least this gomarkdown/markdown#95 seems where they are talking about the performance issue. Though I can judge nothing of Go benchmark details. Using Hugo has virtually nothing to do with using Go.

@jmarca

This comment has been minimized.

Copy link

commented Nov 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.