classification
Title: subprocess.run should alias universal_newlines to text
Type: enhancement Stage: commit review
Components: Library (Lib) Versions: Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: gregory.p.smith Nosy List: andrewclegg, barry, gregory.p.smith, ncoghlan, steve.dower
Priority: normal Keywords: patch

Created on 2017-10-11 10:16 by andrewclegg, last changed 2017-10-23 02:03 by gregory.p.smith. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 4049 merged andrewclegg, 2017-10-19 10:50
Messages (6)
msg304125 - (view) Author: Andrew Clegg (andrewclegg) * Date: 2017-10-11 10:16
Following on from https://bugs.python.org/issue6135

The subprocess module by default returns bytes from subprocess calls. It has a text mode, but this can only be accessed by slightly tangential arguments (setting encoding, errors or universal_newlines).

ncoghlan notes in msg304118 that this is a similar situation to the binary/text mode settings for open(). From the docs " In text mode, if encoding is not specified the encoding used is platform dependent: locale.getpreferredencoding(False) is called to get the current locale encoding"

The universal_newlines argument now effectively just turns on the text mode, however this is not an intuitively and obviously discoverable. So to improve usability, and to mirror the file opening behaviour, subprocess calls should be *explicitly* dual mode (binary/text), and have an explicitly named argument to control this.

My enhancement suggestion is as follows:
* Add a text=True/False argument that is an alias of universal_newlines, to improve the API.
* Clearly document that this implies that the encoding will be guessed, and that an explicit encoding can be given if the guess is wrong

For completeness, the following changes could also be made, although these may be controversial
* Remove/deprecate the universal_newlines argument
* Revert the default mode to text (as in Python 2.7), and have a binary=True argument instead
msg304149 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2017-10-11 15:23
Really, this is just an alias for universal_newlines in Popen.__init__. So we add the parameter and:

+    if text:
+        universal_newlines = True
     self.universal_newlines = universal_newlines

And 99% of the change is making it clear in the docs why we have two arguments with the same meaning.
msg304150 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2017-10-11 15:25
> just an alias

Which I recognise is in the bug title. My point is that the enhancement itself is less relevant than the rationale and the documentation. 

Without a doc patch, there's really nothing to discuss here other than duplicating APIs (which is probably justified, even though it looks like a wart).
msg304158 - (view) Author: Andrew Clegg (andrewclegg) * Date: 2017-10-11 16:22
OK great, I'll get working on a patch.
msg304198 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2017-10-12 02:34
As far as docs phrasing goes, it probably makes sense to frame it as "text" being the preferred argument name in 3.7+, with "universal_newlines" retained indefinitely as a compatibility preserving alias. After all, if that wasn't our intention, we wouldn't be adding the more straightforward alias in the first place.

As a new Py3-only argument, "text" can also be made keyword-only. (The Popen arg list is so long that most folks treat everything other than the first item as keyword-only anyway, but it doesn't hurt to ask the interpreter to enforce that)
msg304774 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2017-10-23 02:01
New changeset 7fed7bd8bb628f0f09c6011871a4ce68afb41b18 by Gregory P. Smith (andyclegg) in branch 'master':
bpo-31756: subprocess.run should alias universal_newlines to text (#4049)
https://github.com/python/cpython/commit/7fed7bd8bb628f0f09c6011871a4ce68afb41b18
History
Date User Action Args
2017-10-23 02:03:49gregory.p.smithsetstatus: open -> closed
resolution: fixed
stage: patch review -> commit review
2017-10-23 02:01:22gregory.p.smithsetmessages: + msg304774
2017-10-22 20:34:58wolmasetnosy: - wolma
2017-10-22 20:22:30wolmasetnosy: + wolma
2017-10-20 20:49:47gregory.p.smithsetassignee: gregory.p.smith

nosy: + gregory.p.smith
2017-10-19 10:50:50andrewcleggsetkeywords: + patch
stage: patch review
pull_requests: + pull_request4018
2017-10-12 02:34:51ncoghlansetmessages: + msg304198
2017-10-11 16:22:08andrewcleggsetmessages: + msg304158
2017-10-11 15:25:45steve.dowersetmessages: + msg304150
2017-10-11 15:23:39steve.dowersetmessages: + msg304149
2017-10-11 13:47:28barrysetnosy: + barry
2017-10-11 10:16:51andrewcleggcreate