Allow one level of balanced parens in link urls #74
Conversation
ariabuckles
left a comment
ariabuckles
left a comment
There was a problem hiding this comment.
Hi again Sergey!
Thanks for this! I've been wanting to address this for ages, and this looks great!
One inline comment around some edge cases I think we might want to consider. If you'd like to add handling for them that would be wonderful, or if you'd prefer I'm happy to merge this and see about adding handling for those cases myself or add separate issues for them.
| var LINK_INSIDE = "(?:\\[[^\\]]*\\]|[^\\[\\]]|\\](?=[^\\[]*\\]))*"; | ||
| var LINK_HREF_AND_TITLE = | ||
| "\\s*<?((?:[^\\s\\\\]|\\\\.)*?)>?(?:\\s+['\"]([\\s\\S]*?)['\"])?\\s*"; | ||
| "\\s*<?((?:\\([^)]*\\)|[^\\s\\\\]|\\\\.)*?)>?(?:\\s+['\"]([\\s\\S]*?)['\"])?\\s*"; |
There was a problem hiding this comment.
Two minor things I think this might not handle yet are:
-
It allows unescaped opening quotes in the match, which results in confusing matches, such as:
[link](http://example.com/test((hi))rendering to<a href="http://example.com/test((hi)">link</a>.- I think we might be able to just add
(to the list of forbidden characters in the parenthesis capture like\\([^()]*\\)to avoid this.
- I think we might be able to just add
-
Escaping the closing
), like:[link](http://example.com/test(hi\)). It's... pretty unlikely that anyone really needs that 😄, but we do support it in other places.- I think something like:
\\((?:[^)\\\\]|\\\\.)*\\)instead of\\([^)]*\\)might let that work (see how the later block uses[^\\s\\\\]|\\\\.to do the same thing).
- I think something like:
|
Hi, Aria! I think it would be better to open separate issues if the current state of things will not make it worse (not sure about that). I'm not sure that I'll have time to come up with solution to the problems in comments very soon. Would be cool to not let this issue hang around even more. But probably it's bad idea if it will make things worse for users. Anyway, you decide. I'll try to find the time, but no promises about that. |
|
Sounds good. I think this will make things better so I'm happy to merge it as-is! Thanks again for fixing this, and sorry for the slow response! |
|
No problem 👍 Can you take a look at my another PR, please? I just rebased it on master. |
|
Yes of course! Thanks for rebasing it! |
2 participants
Fixes #21