search

sindresorhus / linkify-urls

Linkify URLs in a string
MIT License
160 stars 22 forks source link

Trailing punctuation is included in link (periods, question marks) #26

Open karlhorky opened 5 years ago

karlhorky commented 5 years ago

sindresorhus/refined-github#381 introduced adding links to URLs in code, which is a really cool feature!

However, it doesn't follow the same rules as Markdown in Markdown code, such as the case where a link has a period at the end (the link on https://github.com/palmerhq/tsdx/pull/138/files goes to https://github.com/alexreardon/tiny-invariant., with the period):

Screen Shot 2019-08-15 at 17 12 18

I can think of two ways to deal with this off the top of my head (maybe there's a better / easier alternative):

kidonng commented 5 years ago

The regular expression comes from upstream and I think this is expected. There are times when links end with . (e.g. https://www.google.com.). Let alone it could end with other marks like ! :. Better practice is to add a space after the link 😀

karlhorky commented 5 years ago

Better practice is to add a space after the link

I don't think this is actually an argument. If it's just your code or my code, we can discuss this, sure. But we're talking about 3rd-party code on all historical commits on GitHub - can't go around asking people to rewrite their old commits.

Would be interesting to hear more real-world examples for links with punctuation at the end. I'm guessing there aren't many.

I think it's about what would happen more in real-world situations - links with punctuation at the end (which happen... almost never?) or instances of people adding punctuation at the end of links. I would guess there are tons of instances of the second case. So the tradeoff would be to break the URLs in the almost-never case of links with punctuation at the end in order to fix something that happens a lot.

The regular expression comes from upstream

I guess I would probably not suggest PRing this to upstream, since that module does its job nicely and does not have the constraints that refined-github does. I would suggest putting in additional manipulation after the transformation from the upstream module to fix this issue.

cc @sindresorhus because ownership of upstream.

fregante commented 5 years ago

I asked @sindresorhus to move this issue there. It won't be fixed here, especially because technically we're still waiting for Firefox to update to linkify-urls@3. Info in sindresorhus/refined-github#803

karlhorky commented 5 years ago

Ok, if it should be an issue on upstream, then maybe there would be a way it could make sense - a default-off option like stripTrailingPunctuation, for example.