Skip to content

bpo-29704: Fix asyncio.SubprocessStreamProtocol closing #405

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

Merged
merged 5 commits into from
Mar 3, 2017
Merged

bpo-29704: Fix asyncio.SubprocessStreamProtocol closing #405

merged 5 commits into from
Mar 3, 2017

Conversation

sethmlarson
Copy link
Contributor

Was told by @1st1 to open this PR here from python/asyncio. See the original Pull Request for discussion and review: python/asyncio#485

@mention-bot
Copy link

@SethMichaelLarson, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Haypo and @1st1 to be potential reviewers.

@1st1
Copy link
Member

1st1 commented Mar 2, 2017

LGTM. @asvetlov @fafhrd91 want to take a look too?

@1st1
Copy link
Member

1st1 commented Mar 2, 2017

@SethMichaelLarson Do we have an open issue for this on bugs.python.org? If not can you create one?

@sethmlarson
Copy link
Contributor Author

We do not, I will create one now.

@sethmlarson
Copy link
Contributor Author

I created an issue with @BotoX comments and links to issue on python/asyncio http://bugs.python.org/issue29704

@1st1 1st1 changed the title Pull Request from python/asyncio#485 bpo-29704: Fix asyncio.SubprocessStreamProtocol closing Mar 2, 2017
@1st1
Copy link
Member

1st1 commented Mar 2, 2017

Please add a Misc/NEWS entry (to the top of Library section for 3.7.0 alpha)

@sethmlarson
Copy link
Contributor Author

@1st1 Done, is that entry acceptable?

@1st1
Copy link
Member

1st1 commented Mar 2, 2017

Yep. Looking good now.

@1st1 1st1 merged commit 481cb70 into python:master Mar 3, 2017
@1st1
Copy link
Member

1st1 commented Mar 3, 2017

This PR is merged, however, I didn't see that it had some trailing whitespace. Please make sure that your editor strips trailing whitespace.

@1st1
Copy link
Member

1st1 commented Mar 3, 2017

Note: tested this PR with all uvloop functional subprocess tests, everything looks alright.

@sethmlarson
Copy link
Contributor Author

@1st1 Sorry about that! My fault for not noticing. GitHub's web editor doesn't strip whitespace like PyCharm does. :) I'm glad it also works in uvloop, I should probably test asyncio changes with uvloop as well first before submitting.

@1st1
Copy link
Member

1st1 commented Mar 3, 2017

I'm glad it also works in uvloop, I should probably test asyncio changes with uvloop as well first before submitting.

It's not so much about uvloop as it is about some unittests there that test both uvloop & vanilla asyncio. I plan to port some of those tests to CPython to increase coverage.

@1st1
Copy link
Member

1st1 commented Mar 3, 2017

Anyways, thanks for the contribution!

@sethmlarson
Copy link
Contributor Author

@1st1 Would be a good idea to port them, I've got some tests for selectors2 that don't occur in Python's stdlib selectors that might be considered good as well for unique situations.

@sethmlarson sethmlarson deleted the patch-1 branch March 3, 2017 04:51
@1st1
Copy link
Member

1st1 commented Mar 3, 2017

I've got some tests for selectors2 that don't occur in Python's stdlib selectors that might be considered good as well for unique situations.

Please open a PR with those tests, selectors module is very important. For an extra benefit, you'll see your tests running on the full CPython buildbots matrix.

@sethmlarson
Copy link
Contributor Author

Will do, might have to experiment with different Python versions, PEP 475 really split the behavior for anything syscall-related in Python.

@sethmlarson
Copy link
Contributor Author

sethmlarson commented Mar 3, 2017

What is the protocol for tests that behave differently (skip?) on different Python versions? Would they only be put in the version where they should be or is it normal to have tests that only run on Python 3.5+, for example.

@vstinner
Copy link
Member

vstinner commented Mar 3, 2017 via email

@sethmlarson
Copy link
Contributor Author

@Haypo Ah 3.4 is off bug-fix. Makes that a lot easier! :) Disregard my above comment then.
The tests would have to do with selectors.

@vstinner
Copy link
Member

vstinner commented Mar 3, 2017 via email

@fafhrd91
Copy link
Contributor

fafhrd91 commented Mar 3, 2017

@Haypo some rasberypi dev still on 3.4.2
https://home-assistant.io this is still on 3.4.2
I had to ship backported cookies module with aiohttp.

@vstinner
Copy link
Member

vstinner commented Mar 3, 2017 via email

jaraco pushed a commit that referenced this pull request Dec 2, 2022
* Use GH to signify Github handle of the user instead of "@".

"@" usually doesn't give any information about what exactly the handle means
since we don't know it is a Github handle from the commit message.

Changing the pattern to be `GH:` similar to what we do for Pull Requests
with (GH-).

Fixes #270

* Handle removing GH:-style Automerge-Triggered-By

...when automerge label is removed by a core-dev.

Co-authored-by: Abhilash Raj <raj.abhilash1@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants