Skip to content

bpo-35019: Allow ipaddres.IPv4/v6Address in asyncio.create_server#9956

Closed
ghost wants to merge 2 commits into
masterfrom
unknown repository
Closed

bpo-35019: Allow ipaddres.IPv4/v6Address in asyncio.create_server#9956
ghost wants to merge 2 commits into
masterfrom
unknown repository

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Oct 18, 2018

Allow ipaddres.IPv4/v6Address in asyncio.create_server

bpo-35019 - Checked the host for if is instance of IPv4/v6Address and if it is a instance of that get the value with with exploded.

https://bugs.python.org/issue35019

@the-knights-who-say-ni
Copy link
Copy Markdown

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

@tirkarthi
Copy link
Copy Markdown
Member

Thanks for the PR @DamlaAltun . I think this deserves a NEWS entry since this is an enhancement over the existing API adding support for IPv4 and IPv6 objects. You can find more about NEWS entry at https://devguide.python.org/committing/#what-s-new-and-news-entries . You can use blurb to add one.

@ghost
Copy link
Copy Markdown
Author

ghost commented Oct 19, 2018

Thanks, i'll add a news entry.

Copy link
Copy Markdown
Contributor

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

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

I think we should implement something like os.PathLike / __fspath__ but for IP addresses.
Support it in socket module instead of adding a partial support for asyncio (connect API doesn't support ip addresses after the patch is applied for example)

@bedevere-bot
Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be poked with soft cushions!

@ghost
Copy link
Copy Markdown
Author

ghost commented Oct 20, 2018

I think it is big enough for posting to Python-ideas and writing a PEP. If it is possible i want to work on that.

  • An abstract base class in ipaddr module with an abstract method __ipaddr__ and subclasshook.
  • Adding a function to ipaddr for getting given instance's __ipaddr__ value.
  • Adding __ipaddr__ support to IPv4Address and IPv6Address and registering them to IPLike Abstract Base Class.
  • Supporting IPLike objects in asyncio.
  • Updating API / Docs / Glossary

@asvetlov
Copy link
Copy Markdown
Contributor

@DamlaAltun sounds good!

I think we should start from worikng on implementation first and after getting it done we can propose a PEP.
The implementation doesn't need the docs update but robust changes for ipaddress and socket modules. From my vision, no asyncio modification is needed if underlying socket APIs will support objects from ipaddress seamlessly.

Please keep me in the loop and don't afraid to ask for any help.

I'm closing the PR for sake of more generalized solution.

@asvetlov asvetlov closed this Oct 20, 2018
@ghost
Copy link
Copy Markdown
Author

ghost commented Oct 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants