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

Add brackets to the allowed path chars #10

Merged
merged 1 commit into from
Aug 11, 2015
Merged

Add brackets to the allowed path chars #10

merged 1 commit into from
Aug 11, 2015

Conversation

sztanpet
Copy link
Contributor

brackets are widely used in paths, so add them

http://bgp.he.net/search?search[search]=vortex.data.microsoft.com&commit=Search was not matched for example

@sztanpet
Copy link
Contributor Author

this does not make sure the brackets are well formed

@mvdan
Copy link
Owner

mvdan commented Aug 11, 2015

I was worried about parenthesis because they are used often, like in markdown links. Hence why I made xurls only accept them if they matched in the url.

Can you think of any pattern that would make urls be surrounded by brackets? Somethink like [http://foo.bar] that would need to match http://foo.bar instead of http://foo.bar].

@sztanpet
Copy link
Contributor Author

maybe in bbcode? otherwise not really

@mvdan
Copy link
Owner

mvdan commented Aug 11, 2015

I'll merge this and fix the "surrounded by brachets" edge case separately.

Thanks for your help!

mvdan added a commit that referenced this pull request Aug 11, 2015
Add brackets to the allowed path chars
@mvdan mvdan merged commit 0837a76 into mvdan:master Aug 11, 2015
@sztanpet
Copy link
Contributor Author

thank you!
brackets might be an issue with ipv6 addresses too, since they are only valid when they are surrounded by them

@sztanpet sztanpet deleted the brackets branch August 11, 2015 18:06
@mvdan mvdan mentioned this pull request Aug 11, 2015
@mvdan
Copy link
Owner

mvdan commented Aug 12, 2015

@sztanpet done.

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.

None yet

2 participants