Skip to content

bpo-43776: Remove list call from args in Popen repr#25338

Merged
miss-islington merged 6 commits into
python:masterfrom
mpkocher:fix-popen-repr
Apr 28, 2021
Merged

bpo-43776: Remove list call from args in Popen repr#25338
miss-islington merged 6 commits into
python:masterfrom
mpkocher:fix-popen-repr

Conversation

@mpkocher

@mpkocher mpkocher commented Apr 10, 2021 •
edited by miss-islington
Loading

Copy link
Copy Markdown
Contributor

Removes the list call in the Popen repr.

Current implementation:

For cmd = python --version, with shell=True.

<Popen: returncode: None args: ['p', 'y', 't', 'h', 'o', 'n', ' ', '-', '-',...>

For shell=False and args=['python', '--version'], the output is correct:

<Popen: returncode: None args: ['python', '--version']>

With the new changes the repr yields:

For cmd = python --version, with shell=True:

<Popen: returncode: None args: 'python --version'>

For shell=False and args=['python', '--version'], the output:

<Popen: returncode: None args: ['python', '--version']>

https://bugs.python.org/issue43776

Automerge-Triggered-By: GH:gpshead

@mpkocher mpkocher requested a review from gpshead as a code owner April 10, 2021 23:58
@the-knights-who-say-ni

the-knights-who-say-ni commented Apr 10, 2021

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 this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@mpkocher

For legal reasons we need all the people listed to sign the CLA 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 the contribution, we look forward to reviewing it!

@gpshead gpshead added needs backport to 3.9 type-bug An unexpected behavior, bug, or error labels Apr 11, 2021
@gpshead

gpshead commented Apr 11, 2021

Copy link
Copy Markdown
Member

Can you add a regression test case to Lib/test/test_subprocess.py?
Also a NEWS entry via blurb.

@gpshead gpshead self-assigned this Apr 11, 2021
@mpkocher

mpkocher commented Apr 13, 2021

Copy link
Copy Markdown
Contributor Author

@gpshead I've added a test for the new changes as well as a NEWS entry.

Please let me know if you have any suggestions for improvements or if you have comments on the changes.

gpshead
gpshead requested changes Apr 24, 2021
View reviewed changes
Comment thread Lib/test/test_subprocess.py Outdated
Comment thread Misc/NEWS.d/next/Library/2021-04-12-00-00-00.bpo-43776.p14y7a.rst Outdated
@bedevere-bot

bedevere-bot commented Apr 24, 2021

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.

@gpshead

gpshead commented Apr 24, 2021

Copy link
Copy Markdown
Member

Please do the CLA signing dance as describe by the bot above as well. Thanks!

@mpkocher

mpkocher commented Apr 26, 2021

Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again

@bedevere-bot

bedevere-bot commented Apr 26, 2021

Copy link
Copy Markdown

Thanks for making the requested changes!

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

@bedevere-bot bedevere-bot requested a review from gpshead April 26, 2021 18:01
@miss-islington

miss-islington commented Apr 28, 2021

Copy link
Copy Markdown
Contributor

@mpkocher: Status check is done, and it's a success ✅ .

@miss-islington miss-islington merged commit db0c5b7 into python:master Apr 28, 2021
@miss-islington

miss-islington commented Apr 28, 2021

Copy link
Copy Markdown
Contributor

Thanks @mpkocher for the PR 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖

@miss-islington

miss-islington commented Apr 28, 2021

Copy link
Copy Markdown
Contributor

Sorry, @mpkocher, I could not cleanly backport this to 3.9 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker db0c5b786df961785ae8c803f5572ae0c8dadcc7 3.9

@Mariatta

Mariatta commented Jun 2, 2021

Copy link
Copy Markdown
Member

Does this still need backport to 3.9? If not, please remove the label.

@miss-islington

miss-islington commented Jun 3, 2021

Copy link
Copy Markdown
Contributor

Thanks @mpkocher for the PR 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖

@miss-islington

miss-islington commented Jun 3, 2021

Copy link
Copy Markdown
Contributor

Sorry @mpkocher, I had trouble checking out the 3.9 backport branch.
Please backport using cherry_picker on command line.
cherry_picker db0c5b786df961785ae8c803f5572ae0c8dadcc7 3.9

gpshead pushed a commit to gpshead/cpython that referenced this pull request Jun 3, 2021
@mpkocher @gpshead
[3.9] bpo-43776: Remove list call from args in Popen repr (pythonGH-2…
5701fcc 
…5338)

Removes the `list` call in the Popen `repr`.

Current implementation:

For cmd = `python --version`,  with `shell=True`.

```bash
<Popen: returncode: None args: ['p', 'y', 't', 'h', 'o', 'n', ' ', '-', '-',...>
```

For `shell=False` and args=`['python', '--version']`, the output is correct:

```bash
<Popen: returncode: None args: ['python', '--version']>
```

With the new changes the `repr`  yields:

For cmd = `python --version`,  with `shell=True`:

```bash
<Popen: returncode: None args: 'python --version'>
```

For `shell=False` and args=`['python', '--version']`, the output:

```bash
<Popen: returncode: None args: ['python', '--version']>
```

Automerge-Triggered-By: GH:gpshead.
(cherry picked from commit db0c5b7)

Co-authored-by: M. Kocher <michael.kocher@me.com>
@bedevere-bot

bedevere-bot commented Jun 3, 2021

Copy link
Copy Markdown

GH-26510 is a backport of this pull request to the 3.9 branch.

gpshead added a commit that referenced this pull request Jun 3, 2021
@gpshead @mpkocher
[3.9] bpo-43776: Remove list call from args in Popen repr (GH-25338) (G…
5a8ddcc 
…H-26510)

Removes the `list` call in the Popen `repr`.

Current implementation:

For cmd = `python --version`,  with `shell=True`.

```bash
<Popen: returncode: None args: ['p', 'y', 't', 'h', 'o', 'n', ' ', '-', '-',...>
```

For `shell=False` and args=`['python', '--version']`, the output is correct:

```bash
<Popen: returncode: None args: ['python', '--version']>
```

With the new changes the `repr`  yields:

For cmd = `python --version`,  with `shell=True`:

```bash
<Popen: returncode: None args: 'python --version'>
```

For `shell=False` and args=`['python', '--version']`, the output:

```bash
<Popen: returncode: None args: ['python', '--version']>
```

Automerge-Triggered-By: GH:gpshead.
(cherry picked from commit db0c5b7)

Co-authored-by: M. Kocher <michael.kocher@me.com>

Co-authored-by: M. Kocher <michael.kocher@me.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gpshead gpshead gpshead approved these changes

Assignees

@miss-islington miss-islington

Labels

type-bug An unexpected behavior, bug, or error

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@mpkocher @the-knights-who-say-ni @gpshead @bedevere-bot @miss-islington @Mariatta