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

Markdown refactoring? #160

Closed
reillysiemens opened this issue Oct 29, 2017 · 8 comments
Closed

Markdown refactoring? #160

reillysiemens opened this issue Oct 29, 2017 · 8 comments
Labels
Breaking change done in pr Already done in a PR

Comments

@reillysiemens
Copy link
Contributor

Hey, @Keats. I've been using Gutenberg for a couple of months to build my personal site and I'm really impressed with the architecture. While working on my site @RadicalZephyr and I encountered an issue. It seems that when arguments are present in a shortcode with a body the paragraph immediately following is not correctly emitted. Unfortunately we have not yet been able to produce a minimal reproduction yet.

This led us to dive into the Markdown parsing code. We found the code pretty challenging to understand and haven't been able to identify what the issue is. One thing we did notice is that it seems like the series of event handlers implicitly specify a state machine. We think it'd be easier to find the bug if we encode the behavior of the numerous mutable state variables as an explicit state machine (possibly multiple state machines). Would you be open to a pull request that restructures the markdown_to_html function to be less monolithic and hopefully easier to reason about?

reillysiemens added a commit to reillysiemens/zola that referenced this issue Oct 29, 2017
reillysiemens added a commit to reillysiemens/zola that referenced this issue Oct 29, 2017
reillysiemens added a commit to reillysiemens/zola that referenced this issue Oct 29, 2017
@Keats
Copy link
Collaborator

Keats commented Oct 30, 2017

The markdown rendering is a pile of hacks, I'd be happy if you can make it more manageable!

@Keats
Copy link
Collaborator

Keats commented Nov 1, 2017

I released 0.2.2 so you can get started if you want

@Keats
Copy link
Collaborator

Keats commented Nov 3, 2017

It would be great to add testcases based on #165 when you get to it

@reillysiemens
Copy link
Contributor Author

@Keats: Sure thing! I'm planning to spend some time on this this afternoon. 😁

@Keats
Copy link
Collaborator

Keats commented Feb 9, 2018

It might be worth looking at https://github.com/kivikakk/comrak as well since pulldown-cmark doesn't really seem maintained. It's about 1.5x-2x slower but it might be ok.

See https://github.com/Keats/markdown-benches-rs for benches

@reillysiemens
Copy link
Contributor Author

@RadicalZephyr and I have noticed that it's hard to get responses from the pulldown-cmark maintainers, but it would be really worrisome if it was truly unmaintained as rustdoc just removed hoedown in favor of pulldown recently: rust-lang/rust#48274

FWIW, we're continuing to chip away at this every other weekend or so, but it's been slow going because of just how difficult pulldown-cmark's internals are to deal with. The dream of extensible Markdown rendering in Rust hasn't been realized yet... 😅

@Keats
Copy link
Collaborator

Keats commented Feb 20, 2018

@RadicalZephyr and I have noticed that it's hard to get responses from the pulldown-cmark maintainers, but it would be really worrisome if it was truly unmaintained as rustdoc just removed hoedown in favor of pulldown recently: rust-lang/rust#48274

I noticed that as well, it got a new release for the Rust project but it doesn't look like other issues are addressed.

FWIW, we're continuing to chip away at this every other weekend or so, but it's been slow going because of just how difficult pulldown-cmark's internals are to deal with. The dream of extensible Markdown rendering in Rust hasn't been realized yet... sweat_smile

Cool, I was actually going to start on it soon if I didn't hear from it soon. I thought with the preprocessing of shortcodes there wasn't too much need for extending pulldown-cmark though, did I miss something? Comrak gives an AST and is actually respecting all aspects of the spec so manipulating the ast should be fairly easier than the events of pulldown-cmark. Maybe add a bench somewhere to see the actual impact on a full article rendering?

@reillysiemens
Copy link
Contributor Author

With shortcodes being preprocessed that won't be helped by extensible rendering, but I think it's still necessary for anchor tags.

If Comrak gives an AST that sounds preferable. I'll have to check it out! 😁 @RadicalZephyr and I ended up needing to wrap the pulldown-cmark parser in our own lazy AST anyway...

@Keats Keats mentioned this issue May 7, 2018
@Keats Keats added the done in pr Already done in a PR label May 11, 2018
@Keats Keats closed this as completed Aug 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking change done in pr Already done in a PR
Projects
None yet
Development

No branches or pull requests

2 participants