-
Notifications
You must be signed in to change notification settings - Fork 191
Validate ipv6 support #172
Validate ipv6 support #172
Conversation
So far you're encountering test failures, but I'm keeping an eye on the progress. Thanks for doing this work @fredthomsen! |
Had some trouble getting some things to split proper and ended up with a regex. Just wanted to get your thoughts, |
Hmmm, I have pretty mixed feelings about using a regex here. @sigmavirus24, do you have anything useful for this floating around? |
I have a very complicated regular expression that is built up progressively in rfc3986 and is based on the ABNF from RFC 3986Sent from my Android device with K-9 Mail. Please excuse my brevity. |
https://github.com/sigmavirus24/rfc3986/blob/master/rfc3986/misc.py#L83..L114 is what I was talking about |
So, should we either vendor or require that package? |
Sorry I never answered you @Lukasa. You can do either. |
It feels like we should do that then. @fredthomsen, does that sound like something you want to take on? |
Yes. I'll handle this. I'll grab some help from you if needed. On Fri, Oct 23, 2015 at 12:22 PM, Cory Benfield notifications@github.com
-Fred Thomsen |
When making something vendorable, what is the appropriate thing to do in terms of license files, author files, etc? I see other vendorable packages we include have no such files. Also I assume I should pull from the latest release of |
Generally speaking, when vendoring packages you should pull specific releases, include their copyright files in with them, and also add them to a NOTICES file at the root of this repo. In the case of the other projects, I didn't do that because their license and copyright statements are exactly the same as for this project. |
Seems like this library needs a full URI instead of just an IP to do it's job. `
and IPv6 addresses can't be parsed without brackets which is the same as the point above.
I can work around the first problem by prepending schemes; however, the second is that IPv6 addresses will require brackets even when no port is present which seems like a strange restriction to enforce. Is there another way to leverage this module or was there something else you two had in mind when you wanted to incorporate this? |
I think it's ok to require that the IP addresses be surrounded by square brackets. That's an easy enough check to do before passing the IP to the library. |
You can also create a |
…y brackets when passed into the constructor
@sigmavirus24 thanks for the help, I took that approach. @Lukasa, I updated the test cases accordingly to reflect what you said. Pushed the changes, but I am having some issues with pypy failures. |
host, port = hostname.split(':') | ||
return host, int(port) | ||
host, port = to_host_port_tuple(hostname) | ||
return host, port |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may as well condense this to a single logical line now.
else: | ||
port = int(uri.port) | ||
|
||
return ((host, port)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need the double parens here.
@fredthomsen the pypy failures are because you need to add a line to the list of packages otherwise it won't be installed properly. |
Thanks @sigmavirus24. Ok think we're good now. |
🍰 |
Ok, I officially love this and think it's beautiful. Thanks for the fantastic library @sigmavirus24, and thanks for the hard work @fredthomsen. You two are magical people. ✨ 🍰 ✨ |
No description provided.