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

.render_html throws an error when given a parse-only option such as :SMART #96

Closed
roryokane opened this issue Mar 15, 2019 · 1 comment · Fixed by #97
Closed

.render_html throws an error when given a parse-only option such as :SMART #96

roryokane opened this issue Mar 15, 2019 · 1 comment · Fixed by #97

Comments

@roryokane
Copy link

I was trying out CommonMarker 0.18.2, and I noticed that passing the :SMART option to the CommonMarker.render_html method gives an error:

CommonMarker.render_html('"Hello," said the spider.', [:SMART])
Traceback (most recent call last):
       10: from /Users/roryokane/.rbenv/versions/2.6.2/bin/irb:23:in `<main>'
        9: from /Users/roryokane/.rbenv/versions/2.6.2/bin/irb:23:in `load'
        8: from /Users/roryokane/.rbenv/versions/2.6.2/lib/ruby/gems/2.6.0/gems/irb-1.0.0/exe/irb:11:in `<top (required)>'
        7: from (irb):35
        6: from (irb):35:in `rescue in irb_binding'
        5: from /Users/roryokane/.rbenv/versions/2.6.2/lib/ruby/gems/2.6.0/gems/commonmarker-0.18.2/lib/commonmarker.rb:23:in `render_html'
        4: from /Users/roryokane/.rbenv/versions/2.6.2/lib/ruby/gems/2.6.0/gems/commonmarker-0.18.2/lib/commonmarker/config.rb:38:in `process_options'
        3: from /Users/roryokane/.rbenv/versions/2.6.2/lib/ruby/gems/2.6.0/gems/commonmarker-0.18.2/lib/commonmarker/config.rb:38:in `map'
        2: from /Users/roryokane/.rbenv/versions/2.6.2/lib/ruby/gems/2.6.0/gems/commonmarker-0.18.2/lib/commonmarker/config.rb:38:in `block in process_options'
        1: from /Users/roryokane/.rbenv/versions/2.6.2/lib/ruby/gems/2.6.0/gems/commonmarker-0.18.2/lib/commonmarker/config.rb:46:in `check_option'
TypeError (option ':SMART' does not exist for CommonMarker::Config::Render)

However, the conversion to HTML works when I write it the long way with with .render_doc:

CommonMarker.render_doc('"Hello," said the spider.', [:SMART]).to_html
# => "<p>“Hello,” said the spider.</p>\n"

I can see in /lib/commonmarker/config.rb that this must be happening because :SMART is in the Parse enum, but the code is looking in the Render enum. I think the error should not be thrown, so either the code should only look in the Parse enum for options, or it should remove options within the Parse enum from the list of passed options before trying that list against the Render enum.

I confirmed that the error is also thrown when passing .render_html the option :FOOTNOTE, another Parse-only option. It doesn’t throw an error with :UNSAFE, because that option exists in both enums.

@gjtorikian
Copy link
Owner

While I agree this is confusing, I think the error will remain. I will change the error message to indicate that although the config symbol might be correct (eg. :SMART) it is not possible in the render type being provided (eg. does not work in a render_html setting).

Since this wraps a C library, I prefer explicitness over magic, so ejecting non-existent symbols doesn't jive with my philosophy here. When it comes to futzing native code, it is important for the user of the library to define and understanding what they are passing in. I realize this may be less convenient, but it's what I prefer.

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 a pull request may close this issue.

2 participants