Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
miss-islington merged 4 commits into from
May 29, 2019
Merged

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

miss-islington merged 4 commits into from
May 29, 2019

Conversation

@lilydjwg
Copy link
Contributor

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

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

https://bugs.python.org/issue35246

https://bugs.python.org/issue35246

@lilydjwg
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.
@the-knights-who-say-ni
Copy link

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

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!

asvetlov
asvetlov requested changes May 28, 2019
View reviewed changes
Lib/test/test_asyncio/test_subprocess.py Outdated
sys.executable, '-c', 'pass', Path('.'))
awaitp.wait()

self.loop.run_until_complete(execute())
Copy link
Contributor

@asvetlovasvetlovMay 28, 2019

Choose a reason for hiding this comment

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

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

Lib/test/test_asyncio/test_subprocess.py Outdated

deftest_create_subprocess_exec_with_path(self):
asyncdefexecute():
frompathlibimportPath
Copy link
Contributor

@asvetlovasvetlovMay 28, 2019

Choose a reason for hiding this comment

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

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

@bedevere-bot
Copy link

bedevere-bot commented May 28, 2019

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.

@lilydjwg
Copy link
ContributorAuthor

lilydjwg commented May 28, 2019

I have made the requested changes; please review again.

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

@bedevere-bot
Copy link

bedevere-bot commented May 28, 2019

Thanks for making the requested changes!

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

asvetlov
asvetlov requested changes May 28, 2019
View reviewed changes
Copy link
Contributor

@asvetlovasvetlov left a comment

Choose a reason for hiding this comment

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

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

@bedevere-bot
Copy link

bedevere-bot commented May 28, 2019

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!

@asvetlov
Copy link
Contributor

asvetlov commented May 28, 2019

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

@asvetlovasvetlov mentioned this pull request May 28, 2019
Merged
serhiy-storchaka
serhiy-storchaka reviewed May 28, 2019
View reviewed changes
Lib/test/test_asyncio/test_subprocess.py Outdated
deftest_create_subprocess_exec_with_path(self):
asyncdefexecute():
p=awaitsubprocess.create_subprocess_exec(
pathlib.Path(sys.executable), '-c', 'pass')
Copy link
Member

@serhiy-storchakaserhiy-storchakaMay 28, 2019

Choose a reason for hiding this comment

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

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

asvetlov
asvetlov requested changes May 28, 2019
View reviewed changes
Copy link
Contributor

@asvetlovasvetlov left a comment

Choose a reason for hiding this comment

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

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

@lilydjwg
Copy link
ContributorAuthor

lilydjwg commented May 29, 2019

I have made the requested changes; please review again.

@bedevere-bot
Copy link

bedevere-bot commented May 29, 2019

Thanks for making the requested changes!

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

asvetlov
asvetlov approved these changes May 29, 2019
View reviewed changes
Copy link
Contributor

@asvetlovasvetlov left a comment

Choose a reason for hiding this comment

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

Well done!

@miss-islingtonmiss-islington merged commit 744c08a into python:masterMay 29, 2019
@lilydjwglilydjwg deleted the asyncio-subprocess-path-args branch May 29, 2019 07:37
DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
@lilydjwg@DinoV
bpo-35246: fix support for path-like args in asyncio subprocess (pyth…
c7e3365
…onGH-13628) Drop isinstance checks from create_subprocess_exec function and let subprocess module do them. https://bugs.python.org/issue35246https://bugs.python.org/issue35246
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchakaserhiy-storchakaserhiy-storchaka left review comments

@asvetlovasvetlovasvetlov approved these changes

@1st11st1Awaiting requested review from 1st1

@gpsheadgpsheadAwaiting requested review from gpshead

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@lilydjwg@the-knights-who-say-ni@bedevere-bot@asvetlov@serhiy-storchaka@miss-islington