Skip to content

Conversation

@gpshead
Copy link
Member

@gpshead gpshead commented Nov 22, 2020

When the modern text= spelling of the universal_newlines= parameter was added
for Python 3.7, check_output's special case around input=None was overlooked.
So it behaved differently with universal_newlines=True vs text=True. This
reconciles the behavior to be consistent and adds a test to guarantee it.

The documentation already described this check_output input=None behavior
difference vs the other subprocess functions, but it wasn't very clear to readers.
The documentation has been clarified as part of this.

https://bugs.python.org/issue42388

When the modern text= spelling of the universal_newlines= parameter was added
for Python 3.7, check_output's special case around input=None was overlooked.
So it behaved differently with universal_newlines=True vs text=True.  This
reconciles the behavior to be consistent and adds a test to guarantee it.

The documentation already describes this check_output input=None behavior
difference vs the other subprocess functions.  This ensures that documented
behavior remains accurate in all cases.
@izbyshev
Copy link
Contributor

Would it also make sense to clarify the following sentence in the docs?

However, explicitly passing input=None to inherit the parent’s standard input file handle is not supported.

It's not clear to me whether I must not pass input=None at all, or, if I may do that, what the effect is (other than that it's not the inheritance of the parent's stdin). May be it's just me though, English is not my first language.

@github-actions
Copy link

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale Stale PR or inactive for long period of time. label Dec 23, 2020
@gpshead
Copy link
Member Author

gpshead commented Dec 23, 2020

Thanks @izbyshev, I've clarified the docs.

@github-actions github-actions bot removed the stale Stale PR or inactive for long period of time. label Dec 24, 2020
Copy link
Contributor

@izbyshev izbyshev left a comment

Choose a reason for hiding this comment

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

Thanks! If I may nitpick, I suggest an even more clarified change :)

Co-authored-by: Alexey Izbyshev <izbyshev@ispras.ru>
@gpshead gpshead closed this Dec 25, 2020
@gpshead gpshead reopened this Dec 25, 2020
@gpshead gpshead merged commit 64abf37 into python:master Dec 25, 2020
@miss-islington
Copy link
Contributor

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 25, 2020
…honGH-23467)

When the modern text= spelling of the universal_newlines= parameter was added
for Python 3.7, check_output's special case around input=None was overlooked.
So it behaved differently with universal_newlines=True vs text=True.  This
reconciles the behavior to be consistent and adds a test to guarantee it.

Also clarifies the existing check_output documentation.

Co-authored-by: Alexey Izbyshev <izbyshev@ispras.ru>
(cherry picked from commit 64abf37)

Co-authored-by: Gregory P. Smith <greg@krypto.org>
@gpshead gpshead deleted the subprocess-issue42388 branch December 25, 2020 04:57
@bedevere-bot
Copy link

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 25, 2020
…honGH-23467)

When the modern text= spelling of the universal_newlines= parameter was added
for Python 3.7, check_output's special case around input=None was overlooked.
So it behaved differently with universal_newlines=True vs text=True.  This
reconciles the behavior to be consistent and adds a test to guarantee it.

Also clarifies the existing check_output documentation.

Co-authored-by: Alexey Izbyshev <izbyshev@ispras.ru>
(cherry picked from commit 64abf37)

Co-authored-by: Gregory P. Smith <greg@krypto.org>
@bedevere-bot
Copy link

GH-23935 is a backport of this pull request to the 3.8 branch.

miss-islington added a commit that referenced this pull request Dec 25, 2020
…23467)

When the modern text= spelling of the universal_newlines= parameter was added
for Python 3.7, check_output's special case around input=None was overlooked.
So it behaved differently with universal_newlines=True vs text=True.  This
reconciles the behavior to be consistent and adds a test to guarantee it.

Also clarifies the existing check_output documentation.

Co-authored-by: Alexey Izbyshev <izbyshev@ispras.ru>
(cherry picked from commit 64abf37)

Co-authored-by: Gregory P. Smith <greg@krypto.org>
miss-islington added a commit that referenced this pull request Dec 25, 2020
…23467)

When the modern text= spelling of the universal_newlines= parameter was added
for Python 3.7, check_output's special case around input=None was overlooked.
So it behaved differently with universal_newlines=True vs text=True.  This
reconciles the behavior to be consistent and adds a test to guarantee it.

Also clarifies the existing check_output documentation.

Co-authored-by: Alexey Izbyshev <izbyshev@ispras.ru>
(cherry picked from commit 64abf37)

Co-authored-by: Gregory P. Smith <greg@krypto.org>
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
…honGH-23467)

When the modern text= spelling of the universal_newlines= parameter was added
for Python 3.7, check_output's special case around input=None was overlooked.
So it behaved differently with universal_newlines=True vs text=True.  This
reconciles the behavior to be consistent and adds a test to guarantee it.

Also clarifies the existing check_output documentation.

Co-authored-by: Alexey Izbyshev <izbyshev@ispras.ru>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type-bug An unexpected behavior, bug, or error

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants