gjtorikian / commonmarker

Ruby wrapper for the comrak (CommonMark parser) Rust crate
MIT License
416 stars 80 forks source link

Normalize parse and render options #145

Closed phillmv closed 2 years ago

phillmv commented 2 years ago

Hullo!

This PR "normalizes" the option bit mask flags that are made available as parse and render options respectively.

Why?

When you use CommonMarker.render_html we load up :render options, and when you use CommonMarker.render_doc we load up :parse options.

This makes sense; some options are only available to the html renderer, and it doesn't make sense to pass them down to the parser.

However, both render_html and render_doc initialize the parser with the provided options masked integer. Assuming that I'm looking at the right code, both rb_parse_document and rb_markdown_to_html take a VALUE rb_options and pass it directly on to prepare_parser ala

  parser = prepare_parser(rb_options, rb_extensions, $insert_allocator_here);

Consequently, I think every parse option should also be available as a render option.

How?

I looked up each individual flag, sorted them by the order in which they appear in cmark-gfm.h, and then grepped to see whether the option is checked inside a) inlines.c and blocks.c (parse) or b) if inside html.c or a relevant extension (render).

I then copied over all of the parse flags into the render hash and sorted them by the order in which they appear in cmark-gfm.h. I applied the same sorting to the README.

I have left comments accordingly tracing this. Hopefully this PR passes CI!

Cheers,

gjtorikian commented 2 years ago

I think in theory this makes sense, I'll have to think a little more on whether it could potentially affect downstream consumers (i.e. a breaking change). Regardless, though, adding a test to validate the behavior of the "mixing" of options would be great here.

phillmv commented 2 years ago

Considering options were only added & not removed, I suspect we should be OK downstream. I'll add a test to exercise the options.

phillmv commented 2 years ago

Test pass locally, so now CI should be more pliant.

I tweaked some minor tests & added a very basic smoke-test that just exercises receiving every parse & render option. I wasn't sure how else to structure it - writing a test case to exercise every single option is hard! - but I feel reasonably OK with this.

Thank you for your attention Garen, and do let me know if there's anything else you'd like me to add 😄.

gjtorikian commented 2 years ago

Thanks, I'll push this out as a patch release now.