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

Ignore empty title attributes #206

Closed
wants to merge 4 commits into from
Closed

Ignore empty title attributes #206

wants to merge 4 commits into from

Conversation

@winniehell
Copy link
Contributor

@winniehell winniehell commented Dec 30, 2014

Fixes #199.

The fix is not complete though as some tests still fail. This is because conversion is not reversible for empty title attributes:

$ kramdown -ihtml -okramdown <<< '<p><img src="a" alt="b" title="" /></p>'
![b](a)

but

$ kramdown -ikramdown -ohtml <<< '![b](a)'
<p><img src="a" alt="b" /></p>

and therefore assert_equal fails.

Any suggestions? Should I avoid these tests?

@gettalong gettalong self-assigned this Jan 21, 2015
@gettalong
Copy link
Owner

@gettalong gettalong commented Feb 4, 2015

Thanks for the pull request!

Before I can merge it:

  • The #convert_title method should be renamed to #prepare_title or something similar because all #convert_NAME methods indicate the conversion of an kramdown element.
  • If you want to test HTML input, please create a new file with the extension ".htmlinput". At first I didn't know why you added the tests because the normal conversion direction is from text to HTML.

Thank you!

@winniehell
Copy link
Contributor Author

@winniehell winniehell commented Feb 5, 2015

Sorry, i don't get the htmlinput mechanism. 😞

The bugfix does not touch the html-to-html conversion but only the html-to-kramdown conversion. I do not understand how i would write the test such that the backwards conversion (kramdown-to-html) is skipped.

@gettalong
Copy link
Owner

@gettalong gettalong commented Feb 5, 2015

The test cases were written originally written to be in the direction text -> HTML. However, later on other directions were added, like text -> kramdown -> HTML, HTML -> kramdown -> HTML, ...

Normally, when testing the HTML -> kramdown direction, the .text file is compared against the converted .html file. However, sometimes it is explicitly wanted to only test the HTML -> kramdown direction and not the other way around (like it is in this pull request). To facilitate this, you can create a file with the extension .htmlinput and the output file with .text. As the test code sees that there is no .html associated, the kramdown -> HTML direction is not tested.

@winniehell winniehell mentioned this pull request Feb 5, 2015
@winniehell
Copy link
Contributor Author

@winniehell winniehell commented Feb 14, 2015

Seems like I finally fixed it... 🚀

@gettalong gettalong added the bug label Feb 23, 2015
@gettalong
Copy link
Owner

@gettalong gettalong commented Feb 23, 2015

Thanks for the fix! I have squashed the commits and attributed the final one to you.

@gettalong gettalong closed this Feb 23, 2015
gettalong added a commit that referenced this pull request Feb 23, 2015
@winniehell winniehell deleted the winniehell:empty-title-img branch Feb 23, 2015
@winniehell
Copy link
Contributor Author

@winniehell winniehell commented Feb 23, 2015

Thank you for merging! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants