-
-
Notifications
You must be signed in to change notification settings - Fork 85
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
Full support for options and extensions in HtmlRenderer #48
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a couple of comments but nothing terribly blocking. I think the only thing I'd really like to see is a method to parse the @opts
inclusion.
@stream = StringIO.new("".force_encoding("utf-8")) | ||
@need_blocksep = false | ||
@warnings = Set.new [] | ||
@in_tight = false | ||
@in_plain = false | ||
@buffer = '' | ||
@tagfilter = extensions.include?(:tagfilter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's tagfilter
? Is that from LIBERAL_HTML_TAG
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the name of the tagfilter
extension as bundled with cmark-gfm
; we just detect the name of it here and emulate its behaviour.
lib/commonmarker/renderer.rb
Outdated
end | ||
|
||
def sourcepos(node) | ||
if @opts & CommonMarker::Config::Render::SOURCEPOS != 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than the if
indentation I'd rather this bail early:
return "" unless @opts & CommonMarker::Config::Render::SOURCEPOS.zero?
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 That unless
example is pretty messed up (logic backwards, missing parens) but I take your point :p
out('<pre><code') | ||
if node.fence_info && !node.fence_info.empty? | ||
out(' class="language-', node.fence_info.split(/\s+/)[0], '">') | ||
if @opts & CommonMarker::Config::Render::GITHUB_PRE_LANG != 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually now that I see this pattern used a couple of times (one more below!), could we pull it out into a separate method? Like...
def option_included?(option)
@opts & CommonMarker::Config::Render.const_get(option)
end
...
if option_included?(GITHUB_PRE_LANG)
....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const_get
gives me the heeby-jeebies but ruby-enum
defines a value
method which does the same thing.
There was some incomplete support, this is just rounding it off and enabling it on all the same tests we put the normal renderer through.
Most of it doesn't actually need to be switched on or off specially — we rely on the fact that e.g. we'll only get a
strikethrough
ortable_cell
node if the parser had that enabled in the first place. The only switch we do have which is renderer-specific istagfilter
./cc @gjtorikian for review