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

gh-79200: support IPAddress in asyncio.start_server() #17434

Closed
wants to merge 10 commits into from

Conversation

vegerot
Copy link

@vegerot vegerot commented Dec 2, 2019

Add support for new Python3 IPv4Address inet type. This is tested and forwards and backwards compatible with different host types.

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

the-knights-who-say-ni commented Dec 2, 2019

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

Recognized GitHub username

We couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames:

@vegerot

This might be simply due to a missing "GitHub Name" entry in one's b.p.o account settings. This is necessary for legal reasons before we can look at this 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 the contribution, we look forward to reviewing it!

@vegerot
Copy link
Author

vegerot commented Dec 2, 2019

Notes:
I have signed the CLI, but the check yourself link says otherwise.

I did put an issue # in the title.

@vegerot vegerot changed the title 38952: support new Python3 IPv4Address and IPv6Address bpo-38952: support new Python3 IPv4Address and IPv6Address Dec 2, 2019
@vegerot vegerot changed the title bpo-38952: support new Python3 IPv4Address and IPv6Address bpo-38952: support new Python3 IPv4Address Dec 2, 2019
vegerot referenced this pull request in postlund/pyatv Dec 2, 2019
The new interface (DeviceListener) has two methods:

  * connection_lost
  * connection_closed

In case of a problem, e.g. server closes connection, connection_lost is
called. In case of a controlled shutdown (e.g. user calls close),
connection_closed is called instead.
@aeros
Copy link
Member

aeros commented Dec 3, 2019

@vegerot

I have signed the CLI, but the check yourself link says otherwise.

It usually takes around 24-48 hours before the CLI status is updated on GitHub, I'd recommend periodically using the check yourself link to manually refresh the status.

If it's been more than 48 hours since you submitted the CLA, you can reach out to the PSF at either:

  1. contributors@python.org
  2. psf@python.org

Copy link
Contributor

@remilapeyre remilapeyre left a comment

Hi @vegerot, thanks for fixing this! Can you please:

  • change the title of the pull request to "bpo-35019: support IPAddress in asyncio.start_server()"
  • Add a test for this change
  • Add a blurb, you can add one by using https://blurb-it.herokuapp.com/
  • Add a .. versionchanged:: note in Doc/library/asyncio-stream.rst to document this change
  • Add your name to Misc/ACKS

?

Thanks!

@vegerot vegerot changed the title bpo-38952: support new Python3 IPv4Address bpo-35019: support IPAddress in asyncio.start_server() Jun 9, 2020
@brettcannon brettcannon removed their request for review Jun 10, 2020
@vegerot
Copy link
Author

vegerot commented Jun 11, 2020

@ericvsmith @remilapeyre thanks for reviewing my PR so in depth

@aeros
Copy link
Member

aeros commented Jun 11, 2020

@vegerot Please try to avoid force-pushing to the branch after opening the PR, as it messes with the reviews and makes it more difficult to look over the relevant commit history. See the devguide for details.

@vegerot
Copy link
Author

vegerot commented Jun 11, 2020

My bad. I will look over that now. I messed up my fast-forward and ended up with thousands of commits in this PR

@aeros
Copy link
Member

aeros commented Jun 11, 2020

My bad. I will look over that now. I messed up my fast-forward and ended up with thousands of commits in this PR

I've certainly done that before (when I first started contributing). My recommendation is to use git pull upstream master --ff-only for keeping the local master branch up to date, and using the following for updating PR branches that have local commits:

git checkout some-branch
git fetch upstream
git merge upstream/master
git push origin some-branch

See the git bootcamp section of the devguide for details.

@aeros
Copy link
Member

aeros commented Jun 11, 2020

@asvetlov mentioned taking a more generalized approach in the original bpo issue:

I mean if we implement something like PathLike [1] but for IP addresses to socket module and teach all socket APIs to accept IPLike objects along with str -- no asyncio change is required.

Andrew, do you still have the same thoughts on the issue, or should we consider a more simplified solution? I would certainly prefer something like an IPLike approach so we don't have to make changes in future and current asyncio APIs that accept IP addresses, but if that's going to be significantly far away, it may be worthwhile to consider the convenience benefits for just the high-level parts of asyncio (such as asyncio.start_server()).

@vegerot
Copy link
Author

vegerot commented Jun 13, 2020

@aeros
Looking through the dev guide, very helpful. I agree tackling this in a way that doesn't require changes to asyncio would be great. But as a one-line change, no reason we couldn't do both. But I would also be interested in working on another method too 🙂

@vegerot
Copy link
Author

vegerot commented Apr 6, 2021

Any update on this?

@kumaraditya303
Copy link
Contributor

kumaraditya303 commented Dec 8, 2022

Closing as won't fix as discussed on issue.

@vegerot
Copy link
Author

vegerot commented Dec 13, 2022

Closing as won't fix as discussed on issue.

@kumaraditya303 Where is the issue?

@kumaraditya303
Copy link
Contributor

kumaraditya303 commented Dec 13, 2022

Where is the issue?

#79200

@terryjreedy terryjreedy changed the title bpo-35019: support IPAddress in asyncio.start_server() gh-79200: support IPAddress in asyncio.start_server() Dec 13, 2022
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.

None yet

9 participants