Skip to content
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

Support number only urls with valid protocol #311

Merged

Conversation

adamdavidcole
Copy link

This is a potential fix for #300: Numeric URLs without TLD does not link

There are some valid use cases where people use number only urls -- usually for internal tracking systems. Examples:

  • jira://2001930
  • ge-user://69302020
  • tracker://100.200.30

This PR adds support for these links by not requiring the URL to contain at least one word character if it has a valid protocol. It's more of a suggestion, since the codebase seems to take a stance against supporting these links for now.

However, when it comes to validating if the URL has at least one period, the code makes an exception for cases where URL matches have a protocol. I think the same logic should apply to links that are numbers only: if the number sequence is preceded by a protocol, the user likely expects it to be a link.

Let me know what you think!

@gregjacobs
Copy link
Owner

Hey, cool dude, I think you're right and we should allow these links. I think originally we wanted to prevent extraneous links from being created, but I think it's fine to link numbers if they're prefixed with a protocol.

Just one (hopefully quick) request: can you add a test or two in the main autolinker spec file? Just want to make sure that the <a> tags are created correctly around these urls with numbers.

Also, what do you think about the case of linking http://127.0.0.? Thinking that the last period shouldn't be included in the href/anchor tag itself (i.e. output should be: '<a href="http://127.0.0">127.0.0</a>.')

Thoughts?

@adamdavidcole
Copy link
Author

Yup! I'll add that test to the spect.

Also agree that last period shouldn't be included in the href tag itself -- will add a test double checking that as well.

Thanks!

@adamdavidcole
Copy link
Author

Updated the spec file with a couple new tests. Let me know if that looks good to you!

@gregjacobs
Copy link
Owner

Hey this looks great man, sorry for the delay in merging!

@gregjacobs gregjacobs merged commit 7f8a2c3 into gregjacobs:master Feb 27, 2020
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