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 the falsely assigned link for the cases of linking by reference and no link #232

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
2 participants
@wm8120
Contributor

wm8120 commented Dec 4, 2014

Similar to issue #69, the false hyperlinks are assigned when rendering reflink and no link which are automatically hyperlinked by browser at editing. This problem won't happen in plain text mode.

Syndromes
[label]: http://<a href="http://example.com" target="_blank">example.com</a> 

will be rendered into

[label]: <a href="http://[example.com](example.com)" ...>http://example.com</a>

[label]: <a href="http://example.com" target="_blank">http://example.com</a> 

will be rendered into

[label]: <a href="[http://example.com](http://example.com)" ...>http://example.com</a>
Fix

Fix is done by adding the pattern of link reference to the regex in function convertHTMLtoMarkdown .

@wm8120 wm8120 changed the title from fix the to fix the falsely assigned link for the cases of linking by reference and no link Dec 4, 2014

@adam-p

This comment has been minimized.

Show comment
Hide comment
@adam-p

adam-p Dec 8, 2014

Owner

Thanks for the PR. I played around with it and it looks good. I'm also really happy you wrote tests (and added to the explanatory comment).

However...

I was going to offer to integrate the tests into the Mocha framework that MDH uses. And then I realized that you added a copyright line, which I would have to keep forever (because the MIT License requires it, I think). And then I started thinking about code contribution license agreements. And... after hours of reading about this stuff and agonizing, I decided to add a CLA requirement.

And you get to be the guinea pig for it! You can see the instructions here. You basically need to read the agreement and then add a "yes I agree" file to your PR (...and remove the copyright/license line).

If you need any help, please let me know. Otherwise just update your PR and re-submit. (I'll still update the tests. 😃 )

Owner

adam-p commented Dec 8, 2014

Thanks for the PR. I played around with it and it looks good. I'm also really happy you wrote tests (and added to the explanatory comment).

However...

I was going to offer to integrate the tests into the Mocha framework that MDH uses. And then I realized that you added a copyright line, which I would have to keep forever (because the MIT License requires it, I think). And then I started thinking about code contribution license agreements. And... after hours of reading about this stuff and agonizing, I decided to add a CLA requirement.

And you get to be the guinea pig for it! You can see the instructions here. You basically need to read the agreement and then add a "yes I agree" file to your PR (...and remove the copyright/license line).

If you need any help, please let me know. Otherwise just update your PR and re-submit. (I'll still update the tests. 😃 )

@wm8120

This comment has been minimized.

Show comment
Hide comment
@wm8120

wm8120 Dec 8, 2014

Contributor

I updated the PR according to your instruction. BTW, I add the "Yes I agree" file by simply copying your example and replacing the signature line.

Indeed, I need your help to integrate the tests into Mocha. Because I have little knowledge on it.

Contributor

wm8120 commented Dec 8, 2014

I updated the PR according to your instruction. BTW, I add the "Yes I agree" file by simply copying your example and replacing the signature line.

Indeed, I need your help to integrate the tests into Mocha. Because I have little knowledge on it.

@adam-p

This comment has been minimized.

Show comment
Hide comment
@adam-p

adam-p Dec 8, 2014

Owner

I added your tests to Mocha and merged your changes into the development branch.

I also created an issue for this bug you found: #233. (I'm not sure if I needed to do that, but... it seemed to make sense.)

This fix will be in the next release.

Thanks a lot for your help with this.

Owner

adam-p commented Dec 8, 2014

I added your tests to Mocha and merged your changes into the development branch.

I also created an issue for this bug you found: #233. (I'm not sure if I needed to do that, but... it seemed to make sense.)

This fix will be in the next release.

Thanks a lot for your help with this.

@adam-p adam-p closed this Dec 8, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment