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

Fix emphasis escape sequence detection #380

Merged
merged 4 commits into from
May 29, 2014
Merged

Fix emphasis escape sequence detection #380

merged 4 commits into from
May 29, 2014

Conversation

jcheatham
Copy link
Contributor

@robin850 - Fixes issues #214 and #369

@robin850
Copy link
Collaborator

Hey @jcheatham,

Thanks a lot for this patch! ❤️ Could you please add a changelog entry ? Matt or I will merge it then, but I will first have a look on my computer tomorrow to make sure the conformance suite passes on it (c.f. #378 (comment)).

@jcheatham
Copy link
Contributor Author

Alright, changelog updated. Wasn't sure if I should be poking @mattr- for these too. So, this one doesn't include the backslash-hell fix, just the nokogiri test fix in order to get travis to pass (so hopefully this will pass the conformance suite on your computer with no problem ;)

@@ -1,5 +1,9 @@
# Changelog

* Fix emphasis character escape sequence detection while mid-emphasis
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/while/with here, no ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked at it as "during the state of emphasizing", so chose "while"

Copy link
Collaborator

Choose a reason for hiding this comment

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

Your English skill is certainly better than mine anyway, let's keep while.

@robin850
Copy link
Collaborator

Thanks again @jcheatham! The conformance suite is green on my computer. However, #369 doesn't seem to be fixed with this patch, I'm unable to get this test green:

def test_char_escaping_when_highlighting
  markdown = "==attribute\==="
  output = render_with({highlight: true}, markdown)

  html_equal "<p><mark>attribute=</mark></p>\n", output
end

Would you mind having a look please ? :-)

@jcheatham
Copy link
Contributor Author

Hi @robin850, looks like the problem with that case was that = wasn't included in the list of escape characters. I added it and the test (assuming the test above meant "==attribute===" :) The conformance tests failed for me at first, maybe due to some debugging statements I had in place, but I haven't had any failures since... will see what travis has to say about it.

@robin850 robin850 merged commit 96ea6b6 into vmg:master May 29, 2014
robin850 added a commit that referenced this pull request May 29, 2014
Fix emphasis escape sequence detection
@robin850
Copy link
Collaborator

@jcheatham : Yes, my test was wrong anyway. Awesome work! Merging, thanks again! ❤️ 💙 💛 💜 💚

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