Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(4896)

#6135: subprocess seems to use local 8-bit encoding and gives no choice

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 years ago by mark
Modified:
1 year, 1 month ago
Reviewers:
vadmium+py, victor.stinner, steve.dower
CC:
amaury.forgeotdarc, Nick Coghlan, AntoinePitrou, haypo, mark_qtrac.eu, eric.araujo, flormayer_aim.com, Arfrever, r.david.murray, srid, mightyiampresence_gmail.com, andrew.clegg_durham.ac.uk, cjerdonek, devnull_psf.upfronthosting.co.za, Martin Panter, eryksun, steve.dower, berwin22_gmail.com, davispuh
Visibility:
Public.

Patch Set 1 #

Total comments: 2

Patch Set 2 #

Total comments: 5

Patch Set 3 #

Total comments: 1

Patch Set 4 #

Total comments: 16

Patch Set 5 #

Total comments: 19

Patch Set 6 #

Patch Set 7 #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Doc/library/subprocess.rst View 1 2 3 4 5 6 12 chunks +63 lines, -40 lines 1 comment Download
Lib/subprocess.py View 1 2 3 4 5 6 10 chunks +41 lines, -40 lines 3 comments Download
Lib/test/test_subprocess.py View 1 2 3 4 5 6 1 chunk +8 lines, -18 lines 1 comment Download

Messages

Total messages: 8
Martin Panter
http://bugs.python.org/review/6135/diff/1399/Lib/subprocess.py File Lib/subprocess.py (right): http://bugs.python.org/review/6135/diff/1399/Lib/subprocess.py#newcode586 Lib/subprocess.py:586: if encoding is None: What if you wanted the ...
1 year, 5 months ago #1
haypo
http://bugs.python.org/review/6135/diff/18348/Lib/subprocess.py File Lib/subprocess.py (right): http://bugs.python.org/review/6135/diff/18348/Lib/subprocess.py#newcode813 Lib/subprocess.py:813: def getstatusoutput(cmd, **kwargs): You can write: universal_newlines=True, stderr=STDOUT, **kwargs
1 year, 1 month ago #2
Martin Panter
https://bugs.python.org/review/6135/diff/18350/Lib/subprocess.py File Lib/subprocess.py (right): https://bugs.python.org/review/6135/diff/18350/Lib/subprocess.py#newcode34 Lib/subprocess.py:34: encoding=None, errors=None, I suggest making any new parameters keyword-only. ...
1 year, 1 month ago #3
steve.dower
http://bugs.python.org/review/6135/diff/18350/Lib/subprocess.py File Lib/subprocess.py (right): http://bugs.python.org/review/6135/diff/18350/Lib/subprocess.py#newcode34 Lib/subprocess.py:34: encoding=None, errors=None, On 2016/09/06 05:45:04, vadmium wrote: > I ...
1 year, 1 month ago #4
haypo
http://bugs.python.org/review/6135/diff/18358/Doc/library/subprocess.rst File Doc/library/subprocess.rst (right): http://bugs.python.org/review/6135/diff/18358/Doc/library/subprocess.rst#newcode92 Doc/library/subprocess.rst:92: .. versionadded:: 3.5 Need ..versionchanged:: 3.6 Added *encoding* and ...
1 year, 1 month ago #5
steve.dower
http://bugs.python.org/review/6135/diff/18358/Doc/library/subprocess.rst File Doc/library/subprocess.rst (right): http://bugs.python.org/review/6135/diff/18358/Doc/library/subprocess.rst#newcode92 Doc/library/subprocess.rst:92: .. versionadded:: 3.5 On 2016/09/06 22:29:28, haypo wrote: > ...
1 year, 1 month ago #6
haypo
LGTM, just a few minor comments. http://bugs.python.org/review/6135/diff/18366/Lib/subprocess.py File Lib/subprocess.py (right): http://bugs.python.org/review/6135/diff/18366/Lib/subprocess.py#newcode352 Lib/subprocess.py:352: import _bootlocale The ...
1 year, 1 month ago #7
Martin Panter
1 year, 1 month ago #8
https://bugs.python.org/review/6135/diff/18350/Lib/subprocess.py
File Lib/subprocess.py (right):

https://bugs.python.org/review/6135/diff/18350/Lib/subprocess.py#newcode851
Lib/subprocess.py:851: The other arguments are the same as for the Popen
constructor.
On 2016/09/06 17:09:26, steve.dower wrote:
> I'm more inclined just to not allow changing advanced parameters for
> getoutput/getstatusoutput (even though the other helper functions allow it).
> That is, remove **kwargs completely from these two. Thoughts?

Sounds good to me; that is already the status quo :)

https://bugs.python.org/review/6135/diff/18366/Doc/library/subprocess.rst
File Doc/library/subprocess.rst (right):

https://bugs.python.org/review/6135/diff/18366/Doc/library/subprocess.rst#new...
Doc/library/subprocess.rst:73: If *encoding* or *errors* are specified, file
objects for stdin, stdout and
or universal_newlines=True

https://bugs.python.org/review/6135/diff/18366/Lib/subprocess.py
File Lib/subprocess.py (right):

https://bugs.python.org/review/6135/diff/18366/Lib/subprocess.py#newcode1780
Lib/subprocess.py:1780: if self.universal_newlines:
Either update this variable, or add error, encoding to the check

https://bugs.python.org/review/6135/diff/18366/Lib/subprocess.py#newcode1800
Lib/subprocess.py:1800: if self.universal_newlines and input is not None:
Maybe also needs updating (I haven’t investigated so closely)
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld 894c83f36cb7