-
Notifications
You must be signed in to change notification settings - Fork 225
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
Allows you to use :escape_html as documented #352
base: master
Are you sure you want to change the base?
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.
This seems reasonable, but before merging this requires two changes:
- It needs to be updated so conflicts can be fixed
test/tilt_commonmarkertemplate_test.rb
should be updated to include tests for the correct behavior (which should presumably be broken before this PR, and fixed after this PR).
!options[option] | ||
else | ||
options[option] | ||
end |
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.
parse_options
/render_options
setup code is similar enough and now complex enough to warrant extracting into a method. However, that can be done later if you don't want to do it in this PR.
} | ||
INVERT_OPTIONS = [ | ||
:escape_html, | ||
] |
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.
Unless you expect additional inverted options, I would avoid using a constant for this single value. However, that change can be done later.
@@ -16,8 +20,9 @@ class CommonMarkerTemplate < Template | |||
:GITHUB_PRE_LANG, | |||
:HARDBREAKS, | |||
:NOBREAKS, | |||
:SAFE, |
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.
According to the current source code, SAFE was removed in 0.18.0. Does removing this break compatibility with CommonMarker <0.18.0?
See https://github.com/rtomayko/tilt/blob/master/docs/TEMPLATES.md#escape_html--truefalse
Supersedes #350