GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — bpo-35246: fix support for path-like args in asyncio subprocess by lilydjwg · Pull Request #13628 · python/cpython

Conversation

lilydjwg commented May 28, 2019 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

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

https://bugs.python.org/issue35246

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          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

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

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

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!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot added the awaiting review label

May 28, 2019

asvetlov requested changes

May 28, 2019

View reviewed changes

Comment thread

Lib/test/test_asyncio/test_subprocess.py Outdated

		sys.executable, '-c', 'pass', Path('.'))
		await p.wait()

		self.loop.run_until_complete(execute())

asvetlov May 28, 2019

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/test/test_asyncio/test_subprocess.py Outdated


		def test_create_subprocess_exec_with_path(self):
		async def execute():
		from pathlib import Path

asvetlov May 28, 2019

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bedevere-bot removed the awaiting review label

May 28, 2019

bedevere-bot commented May 28, 2019

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot added the awaiting changes label

May 28, 2019


          improve test_subprocess

b69e116

lilydjwg commented May 28, 2019

Copy link

Copy Markdown

Contributor Author

I have made the requested changes; please review again.

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot added awaiting change review and removed awaiting changes labels

May 28, 2019

bedevere-bot commented May 28, 2019

Copy link

Copy Markdown

Thanks for making the requested changes!

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

asvetlov left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this 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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bedevere-bot added awaiting changes and removed awaiting merge labels

May 28, 2019

bedevere-bot commented May 28, 2019

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

asvetlov commented May 28, 2019

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

Comment thread

Lib/test/test_asyncio/test_subprocess.py Outdated

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

serhiy-storchaka May 28, 2019

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

asvetlov and eamanu reacted with thumbs up emoji

All reactions

👍 2 reactions

asvetlov requested changes

May 28, 2019

View reviewed changes

asvetlov left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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

lilydjwg commented May 29, 2019

Copy link

Copy Markdown

Contributor Author

I have made the requested changes; please review again.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot added awaiting change review and removed awaiting changes labels

May 29, 2019

bedevere-bot commented May 29, 2019

Copy link

Copy Markdown

Thanks for making the requested changes!

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

asvetlov approved these changes

May 29, 2019

View reviewed changes

asvetlov left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Well done!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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

DinoV pushed a commit to DinoV/cpython that referenced this pull request

Jan 14, 2020


          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/issue35246


https://bugs.python.org/issue35246

Reviewers

serhiy-storchaka

serhiy-storchaka left review comments

asvetlov

asvetlov approved these changes

1st1

Awaiting requested review from 1st1

gpshead

Awaiting requested review from gpshead

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

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

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

lilydjwg commented May 28, 2019 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.