Skip to content

Conversation

@OscarBataille
Copy link

No description provided.

@gregjacobs
Copy link
Owner

Hey Oscar, thanks for the contribution. Can you add a test or two that shows URLs that should be linked which currently aren't?

// Allow optional path, query string, and hash anchor, not ending in the following characters: "?!:,.;"
// http://blog.codinghorror.com/the-problem-with-urls/
urlSuffixRegex = new RegExp( '[/?#](?:[' + alphaNumericAndMarksCharsStr + '\\-+&@#/%=~_()|\'$*\\[\\]?!:,.;\u2713]*[' + alphaNumericAndMarksCharsStr + '\\-+&@#/%=~_()|\'$*\\[\\]\u2713])?' );
urlSuffixRegex = new RegExp( '[/?#](?:[' + alphaNumericAndMarksCharsStr + '\\-\\^+&@#/%=~_()|\'$*\\[\\]?!:,.;\u2713]*[' + alphaNumericAndMarksCharsStr + '\\-\\^+&@#/%=~_()|\'$*\\[\\]\u2713])?' );
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just fyi, the ^ character doesn't need to be escaped in a regular expression character class unless it is the first character in the character class

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@OscarBataille
Copy link
Author

Just added the tests

@gregjacobs
Copy link
Owner

Hey @OscarBataille. Sorry for the late reply on this! Thanks for the tests so far. Can you add just a couple tests in autolinker.spec.ts too which check the full link generation?

Btw, do you have a real URL where ^ is used? I'm wondering if we may have to URL encode this character as it is not a "safe" URL character.

Thanks,
Greg

@OscarBataille
Copy link
Author

Hey @gregjacobs ,

Indeed https://www.ietf.org/rfc/rfc1738.txt says that ^ is an unsafe character. Is there already some way to encode URL characters or does it need to be implemented ?

I don't have access to a public URL where this character is used because we encountered the problem on a internal URL.

Thanks,
Oscar

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants