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

Normalize parse and render options #145

Merged
merged 4 commits into from Sep 3, 2021

Conversation

phillmv
Copy link
Collaborator

@phillmv phillmv commented Aug 31, 2021

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,

…se or render step, i.e. every option available in parse is also available in render step.
@@ -7,23 +7,28 @@ module Config
OPTS = {
parse: {
DEFAULT: 0,
SOURCEPOS: (1 << 1),
Copy link
Collaborator Author

@phillmv phillmv Aug 31, 2021

Choose a reason for hiding this comment

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

NEW parse option:

As far as I can tell, we check for SOURCEPOS in inlines.c, and if the flag is set we annotate the node with sourcepos info, thereby allowing someone implementing their own renderer to also add sourcepos info to their output.

@@ -7,23 +7,28 @@ module Config
OPTS = {
parse: {
DEFAULT: 0,
SOURCEPOS: (1 << 1),
UNSAFE: (1 << 17)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To keep it inline with the deprecated SAFE flag, cmark-gfm.h defines UNSAFE after 1 << 3, so I've sorted it here accordingly.

Copy link
Owner

Choose a reason for hiding this comment

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

Looks like a comma is missing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

🤦

NOBREAKS: (1 << 4),
VALIDATE_UTF8: (1 << 9),
Copy link
Collaborator Author

@phillmv phillmv Aug 31, 2021

Choose a reason for hiding this comment

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

I copied over all of the parse options and resorted the render hash. As described in the PR description, since the render method also initializes the parser with the same options integer, all of the parse options should be available as render options as well.

@gjtorikian
Copy link
Owner

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
Copy link
Collaborator Author

phillmv commented Aug 31, 2021

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

@phillmv
Copy link
Collaborator Author

phillmv commented Sep 1, 2021

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
Copy link
Owner

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

@gjtorikian gjtorikian merged commit 826048a into gjtorikian:main Sep 3, 2021
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.

None yet

2 participants