bpo-35246: fix support for path-like args in asyncio subprocess by lilydjwg · Pull Request #13628 · python/cpython · GitHub

Drop isinstance checks from create_subprocess_exec function and let
subprocess module do them.

https://bugs.python.org/issue35246

drop isinstance checks from create_subprocess_exec function and let subprocess module do them.

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).

Our records indicate we have not received your 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.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

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

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

please add self.assertIsNone(loop.run_until_complete(...)) check to emphasize that we checked that the test passed correctly.

Move the import to the top of the module.
Use import pathlib form and pathlib.Path for consistency.

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.

I have made the requested changes; please review again.

BTW the Windows CI errors should be resolved by #5914.

Thanks for making the requested changes!

@asvetlov: please review the changes made to this pull request.

Please take a look on failed test on Windows: https://ci.appveyor.com/project/python/cpython/builds/24869318

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!

Sorry, didn't read your last message.

Let's postpone merging for a while.
If #5914 will be merged soon you'll do sync with the master and I'll merge.
Otherwise we have to add tests skipping or expectedFailure() on Windows

If we test for the support of the path protocol it is better to use test.support.FakePath instead of pathlib.Path.

#5914 has been merged.
Please sync with master and address @serhiy-storchaka suggestion about test.support.FakePath.
Rebase is not necessary, git merge master is enough. We squash all commits on merging anyway.

…s-path-args

I have made the requested changes; please review again.

Thanks for making the requested changes!

@asvetlov: please review the changes made to this pull request.

Well done!

…onGH-13628) Drop isinstance checks from create_subprocess_exec function and let subprocess module do them. https://bugs.python.org/issue35246 https://bugs.python.org/issue35246

bpo-35246: fix support for path-like args in asyncio subprocess

d7dc435

drop isinstance checks from create_subprocess_exec function and let subprocess module do them.

lilydjwg requested review from 1st1, asvetlov and gpshead as code owners May 28, 2019 15:27

the-knights-who-say-ni added the CLA not signed label May 28, 2019

bedevere-bot added the awaiting review label May 28, 2019

asvetlov requested changes May 28, 2019

View reviewed changes

bedevere-bot removed the awaiting review label May 28, 2019

bedevere-bot added the awaiting changes label May 28, 2019

improve test_subprocess

b69e116

bedevere-bot added awaiting change review and removed awaiting changes labels May 28, 2019

asvetlov approved these changes May 28, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels May 28, 2019

asvetlov requested changes May 28, 2019

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting merge labels May 28, 2019

asvetlov mentioned this pull request May 28, 2019

bpo-31961: Fix support of path-like executables in subprocess. #5914

Merged

serhiy-storchaka reviewed May 28, 2019

View reviewed changes

asvetlov requested changes May 28, 2019

View reviewed changes

lilydjwg added 2 commits May 29, 2019 14:04

Merge remote-tracking branch 'upstream/master' into asyncio-subproces…

d517ded

…s-path-args

use test.support.FakePath instead of pathlib.Path

e337a71

the-knights-who-say-ni removed the CLA not signed label May 29, 2019

the-knights-who-say-ni added the CLA signed label May 29, 2019

bedevere-bot added awaiting change review and removed awaiting changes labels May 29, 2019

asvetlov approved these changes May 29, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels May 29, 2019

asvetlov added the 🤖 automerge label May 29, 2019

miss-islington merged commit 744c08a into python:master May 29, 2019

bedevere-bot removed the awaiting merge label May 29, 2019

lilydjwg deleted the asyncio-subprocess-path-args branch May 29, 2019 07:37

+                  def test_create_subprocess_exec_with_path(self):
+                      async def execute():
+                          p = await subprocess.create_subprocess_exec(
+                              pathlib.Path(sys.executable), '-c', 'pass')

+                              sys.executable, '-c', 'pass', Path('.'))
+                          await p.wait()
+                      self.loop.run_until_complete(execute())

bpo-35246: fix support for path-like args in asyncio subprocess by lilydjwg · Pull Request #13628 · python/cpython · GitHub

Uh oh!

Uh oh!

bpo-35246: fix support for path-like args in asyncio subprocess - #13628

bpo-35246: fix support for path-like args in asyncio subprocess#13628miss-islington merged 4 commits intopython:masterpython/cpython:masterfrom lilydjwg:asyncio-subprocess-path-argslilydjwg/cpython:asyncio-subprocess-path-argsCopy head branch name to clipboard

Conversation

lilydjwg commented May 28, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented May 28, 2019

Uh oh!

asvetlov May 28, 2019

Choose a reason for hiding this comment

Uh oh!

asvetlov May 28, 2019

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented May 28, 2019

Uh oh!

lilydjwg commented May 28, 2019

Uh oh!

bedevere-bot commented May 28, 2019

Uh oh!

asvetlov left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented May 28, 2019

Uh oh!

asvetlov commented May 28, 2019

Uh oh!

serhiy-storchaka May 28, 2019

Choose a reason for hiding this comment

Uh oh!

asvetlov left a comment

Choose a reason for hiding this comment

Uh oh!

lilydjwg commented May 29, 2019

Uh oh!

bedevere-bot commented May 29, 2019

Uh oh!

asvetlov left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bpo-35246: fix support for path-like args in asyncio subprocess#13628
miss-islington merged 4 commits into
python:masterfrom
lilydjwg:asyncio-subprocess-path-args

lilydjwg commented May 28, 2019 •
edited by bedevere-bot
Loading