sindresorhus / linkify-urls

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

Support `,` in the URL path #13

Closed tikotzky closed 6 years ago

tikotzky commented 6 years ago

Currently if a urls with commas are truncated at the first comma for example

linkifyUrls('https://sindresorhus.com/?id=foo,bar')
// returns 
'<a href="https://sindresorhus.com/?id=foo">https://sindresorhus.com/?id=foo</a>,bar'

This PR adds a comma to the list of allowed path characters in the urlRegex.

tikotzky commented 6 years ago

Was bumping into this issue when using refined-github

tikotzky commented 6 years ago

I modified the regex to not include commas if they are followed by a space. This means that currently

'https://sindresorhus.com/?id=foo,bar,'
// would become
'<a href="https://sindresorhus.com/?id=foo,bar">https://sindresorhus.com/?id=foo,bar,</a>'
// since the , is not followed by a space

and

'https://sindresorhus.com/?id=foo,bar,,'
// would become
'<a href="https://sindresorhus.com/?id=foo">https://sindresorhus.com/?id=foo, bar,</a>,'
// since the , is followed by another ,

I can modify the regex to exclude commas in those 2 scenarios, just not sure how complicated you want the regex to become.

tikotzky commented 6 years ago

here is a railroad diagram showing the regex as it is currently in this PR regexplained 2018-01-16 11-09-00

you can view the railroad diagram here

tikotzky commented 6 years ago

If you want to disallow commas followed by commas or at the end of the line, the railroad diagram would look like this. regexplained 2018-01-16 11-10-50 link here

fregante commented 6 years ago

commas followed by commas

I think that's rare enough to not even be considered; I don't think writing http://url,, makes much sense in the first place.