Python3 support #46
Python3 support #46
Conversation
| @@ -218,7 +222,7 @@ def __init__(self, app, transport): | |||
|
|
|||
| def get_node(self, nodeid, address=None): | |||
| "return node or create new, update address if supplied" | |||
| assert isinstance(nodeid, str) | |||
| assert isinstance(nodeid, bytes) | |||
| assert len(nodeid) == 512 / 8 | |||
There was a problem hiding this comment.
Just noticed this isn't using floored division //
There was a problem hiding this comment.
Yep, I did not change it because the result will always be a whole number (where float == int). Will fix it.
| @@ -218,7 +222,7 @@ def __init__(self, app, transport): | |||
|
|
|||
| def get_node(self, nodeid, address=None): | |||
| "return node or create new, update address if supplied" | |||
| assert isinstance(nodeid, str) | |||
| assert isinstance(nodeid, bytes) | |||
There was a problem hiding this comment.
This statement isn't exactly equivalent in 2/3 since it will return True for both byte and text strings in python 2 but only true for byte strings in python3 .
There was a problem hiding this comment.
Since str == bytes in Python 2, I guess this is want we want. What error condition do you have in mind?
There was a problem hiding this comment.
Nevermind. This isn't a problem. I shouldn't review pull requests prior to having my morning ☕
|
@janx @pipermerriam Guys, from my perspective this is completed. Is there anything that you are still expecting? Any other suggestions? |
| @@ -62,7 +66,7 @@ def __init__(self, ip, udp_port, tcp_port=0, from_binary=False): | |||
| # We only want v4 or v6 addresses | |||
| # see https://docs.python.org/2/library/socket.html#socket.getaddrinfo | |||
| ips = [ | |||
| unicode(ai[4][0]) | |||
| str(ai[4][0]) if PY3 else unicode(ai[4][0]) | |||
There was a problem hiding this comment.
The use of unicode here bugs me. I'm not sure it's exactly a problem, but unicode isn't defined in python 3 so it's weird to have it inlined even though it won't execute in the py3 environment.
This is more of a nitpick but this would be cleaner to just have a force_text function and remove the if/else. Again, just a nitpick. This is totally functional as is.
There was a problem hiding this comment.
I didn't remember why this was required but now checking again, it seems that ipaddress.ip_address only accepts unicode!
https://github.com/phihag/ipaddress/blob/master/ipaddress.py#L159
There was a problem hiding this comment.
Btw, since most of our helpers come from rlp.utils, the right place to add an force_text, force_unicode or similar would be there, but I would prefer to do it later.
|
@utzig looks good. I left a bunch of comments but I think all of them were just nitpick/stylistic things that don't need to stand in the way of getting this merged. |
|
@pipermerriam Thanks a lot, that was thorough! I will check the issues you found as soon as I get some free time. |
1 similar comment
2 similar comments
|
@pipermerriam Hey! Is there anything still blocking this from being merged? |
|
Was good to go last time I looked it over. I can take another look later On Tue, Oct 11, 2016, 6:25 AM Fabio Utzig notifications@github.com wrote:
|
No description provided.