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

whitelist options accepted by pandoc #23

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

mfenner
Copy link

@mfenner mfenner commented Dec 17, 2016

This pull requests whitelists the options sent to pandoc instead of raising a RuntimeError. This makes it much easier to integrate pandoc-ruby with other libraries, specifically tilt (rtomayko/tilt#297), which in turn is used by other libraries. The whitelist can be overridden with the :ignore_whitelist option. An alternative would be to make the whitelist opt-in rather opt-out with a :use_whitelist option, but I feel that the default behavior could be to ignore unknown options rather than raising a RuntimeError.

@xwmx
Copy link
Owner

xwmx commented Dec 20, 2016

Hi Martin,

Thanks for the contribution! After looking into this I want to include some additional notes about this issue, along with my comments.

Just to document the issue here (correct me if I get anything wrong): Middleman is adding support for Tilt 2 which will make it easier to use Pandoc to process Markdown documents. The issue is that Middleman passes several options through Tilt and then PandocRuby that are not compatible with Pandoc, which triggers an error.

After looking at the issue, my current impression is that this would be better implemented by somehow handling errors from Pandoc rather than hardcoding pandoc option names. Handling errors would make the solution more generalized and useful outside of this specific issue, would avoid requiring maintaining a local list of pandoc options, and would make it easier to understand the purpose of the additional code in the long term.

Additionally, I'm reluctant to suppress pandoc errors like this by default, particularly for a specialized use case that would likely not be obvious to others debugging their own programs. Invalid options are invalid and considered errors by the pandoc executable, and that information is important for developers who are working with Pandoc. For example, if a developer made a typo that caused an invalid option to trigger an error from pandoc, that error should probably be passed along through PandocRuby.

I'm not sure what the best way to implement this error handling would be, but looking at the behavior of the pandoc executable might provide some guidance.

When pandoc is called with an invalid argument, it returns with a status of '2' and prints an error message:

❯ pandoc --invalid-option
pandoc: unrecognized option `--invalid-option'
Try pandoc --help for more information.
❯ echo $?
2

pandoc also emits other errors. For example, an invalid reader will trigger the following:

❯ pandoc --from invalid_reader
pandoc: Unknown reader: invalid_reader
❯ echo $?
7

This error information could potentially be used to trigger a more appropriate exception class than just RuntimeError, and those exceptions could then be easier to handle and work with. However, I'm not sure whether this would solve or, at least, help solve the issue you are encountering.

Regarding the specific issue with Middleman, I'm only superficially looking through the Middleman codebase, but it looks like it's structured with Renderers, including several separate renderer classes for different Markdown engines:

https://github.com/middleman/middleman/blob/master/middleman-core/lib/middleman-core/renderers

I haven't looked closely to see exactly how the renderer classes are used, but perhaps the issue of the invalid options from Middleman would be better resolved through a new renderer in Middleman, similar to the renderers for other markdown engines.

I'll try to think a bit more about this.

@mfenner
Copy link
Author

mfenner commented Dec 20, 2016

William,

thanks for the long and thoughtful response. You described the issue pretty well. I encountered the same issue when I was writing a Ruby gem last week (https://rubygems.org/gems/cirneco/) which is not using tilt or middleman, but providing a command line tool that among other things uses pandoc to render markdown. My solution was to only pass options understood by pandoc to pandoc-ruby.

I am not sure how a different markdown renderer would solve this, as we still have to make a decision about which options to pass on to pandoc-ruby, which again probably means a whitelist or blacklist approach. I would think that doing this closer to pandoc in pandoc-ruby is more flexible, as it not only works for middleman.

I see your point about not suppressing errors by default. The challenge I see is to distinguish options that were intended for pandoc from options that had not been removed and were originally intended for something else. That is why I added the :ignore_whitelist option. Maybe it would be safer to make the whitelisting opt-in, and use it only when a :use_whitelist option is set. This shouldn't affect current implementations using pandoc-ruby. I don't think that the options used by pandoc change so frequently, although some of them are not available in all versions.

The only other option I see is to add a --ignore-unknown-commands option to Pandoc directly. I am happy to ask on the Pandoc mailing list whether I missed this option and what the thinking is about adding that option to Pandoc.

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.

2 participants