Skip to content
This repository was archived by the owner on Nov 23, 2017. It is now read-only.

Conversation

ajdavis
Copy link

@ajdavis ajdavis commented May 23, 2016

loop.getaddrinfo accepts service names like "http" for port.

@ajdavis
Copy link
Author

ajdavis commented May 23, 2016

Fixes #353 .

port = int(port)
except ValueError:
# Might be a service name like "http".
port = socket.getservbyname(port)
Copy link
Member

Choose a reason for hiding this comment

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

This works only when port is an str, right? How does the port=b'http' test pass?

Copy link
Author

Choose a reason for hiding this comment

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

Good catch! Fixed.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see. But why don't we just convert bytes to str?

socket.getaddrinfo('127.0.0.1', b'http') works so should our code.

@ajdavis ajdavis force-pushed the getaddrinfo-servname branch from f721972 to 3ba5348 Compare May 23, 2016 21:30
@@ -104,8 +104,18 @@ def _ipaddr_info(host, port, family, type, proto):

if port in {None, '', b''}:
Copy link
Member

Choose a reason for hiding this comment

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

While you're here, could you please fix a BytesWarning:

Please replace if port in {None, '', b''}: with if port is None:;
add if port == '': port = 0 else: to the elif isinstance(port, str),
do the same for bytes.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

@1st1
Copy link
Member

1st1 commented Jun 2, 2016

Merged by hand in 2959a97

Thanks a lot, Jesse.

@1st1 1st1 closed this Jun 2, 2016
@1st1
Copy link
Member

1st1 commented Jun 2, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants