Skip to content

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

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
merged 6 commits into from
Apr 28, 2021
Merged

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

merged 6 commits into from
Apr 28, 2021

Conversation

mpkocher
Copy link
Contributor

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

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
Copy link

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 only security fixes type-bug An unexpected behavior, bug, or error labels Apr 11, 2021
@gpshead
Copy link
Member

gpshead commented Apr 11, 2021

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
Copy link
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
@bedevere-bot
Copy link

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
Copy link
Member

gpshead commented Apr 24, 2021

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

@mpkocher
Copy link
Contributor Author

I have made the requested changes; please review again

@bedevere-bot
Copy link

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
Copy link
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
Copy link
Contributor

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

@miss-islington
Copy link
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
Copy link
Member

Mariatta commented Jun 2, 2021

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

@gpshead gpshead added needs backport to 3.9 only security fixes and removed needs backport to 3.9 only security fixes labels Jun 3, 2021
@miss-islington
Copy link
Contributor

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

@miss-islington
Copy link
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 <[email protected]>
@bedevere-bot bedevere-bot removed the needs backport to 3.9 only security fixes label Jun 3, 2021
@bedevere-bot
Copy link

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 <[email protected]>

Co-authored-by: M. Kocher <[email protected]>
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