bpo-31756: subprocess.run should alias universal_newlines to text#4049
bpo-31756: subprocess.run should alias universal_newlines to text#4049gpshead merged 3 commits intopython:masterfrom
Conversation
Adds text=False as a keyword-only argument to subprocess.Popen. Introduces a Popen attribute text_mode and sets this based on encoding/errors/universal_newlines/text arguments.
|
|
||
| .. versionchanged:: 3.7 | ||
|
|
||
| Added *text* parameter, as alias of *universal_newlines* |
There was a problem hiding this comment.
Added the text parameter, as a more understandable alias of universal_newlines.
| startupinfo=None, creationflags=0, | ||
| restore_signals=True, start_new_session=False, | ||
| pass_fds=(), *, encoding=None, errors=None): | ||
| pass_fds=(), *, encoding=None, errors=None, text=False): |
There was a problem hiding this comment.
Change the defaults for universal_newlines and text from False to None.
and in the code below, raise an error when both are supplied but disagree:
if (text is not None and universal_newlines is not None
and bool(universal_newlines) != bool(text)):
raise SubprocessError('Cannot disambiguate when both text '
'and universal_newlines are supplied but different. '
'Pass one or the other.')| self.stderr = None | ||
| self.pid = None | ||
| self.returncode = None | ||
| self.universal_newlines = universal_newlines |
There was a problem hiding this comment.
The universal_newlines instance attribute is part of the public API of the class (even though not documented).
We should keep the name around. Even if it is turned into a property with a setter and getter that ultimately sets/returns the value of the new self.text_mode attribute.
|
|
||
| communicate() returns a tuple (stdout, stderr). These will be | ||
| bytes or, if self.universal_newlines was True, a string. | ||
| bytes or, if self.universal_newlines/self.text was True, a string. |
|
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 |
Check for any contradictions between universal_newlines and text arguments.
|
Thanks for your feedback. I've made the changes you suggested, and had another go at the documentation for Popen/run/communicate to make it a bit more consistent. I have made the requested changes; please review again. |
|
Thanks for making the requested changes! @gpshead: please review the changes made to this pull request. |
|
thanks for this, |
Adds text=False as a keyword-only argument to subprocess.Popen.
Introduces a Popen attribute text_mode and sets this based on
encoding/errors/universal_newlines/text arguments.
https://bugs.python.org/issue31756