classification
Title: Docstring of the subprocess module should be cleaned up
Type: enhancement Stage: resolved
Components: Documentation Versions: Python 3.7, Python 3.6, Python 3.5, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: docs@python Nosy List: Mariatta, berker.peksag, docs@python, ezio.melotti, martin.panter, python-dev, rbcollins, terry.reedy, tim.mitchell
Priority: normal Keywords: easy, patch

Created on 2016-01-30 05:18 by Antony.Lee, last changed 2016-10-26 04:55 by martin.panter. This issue is now closed.

Files
File name Uploaded Description Edit
subprocess-docs.patch tim.mitchell, 2016-10-18 03:58 patch
subprocess2.patch tim.mitchell, 2016-10-19 01:25 updated patch v2 review
subprocess3.patch tim.mitchell, 2016-10-20 20:01 patch v3 review
subprocess3-3.5.patch martin.panter, 2016-10-22 00:55 review
subprocess3-2.7.patch martin.panter, 2016-10-22 00:55 review
Messages (13)
msg259238 - (view) Author: Antony Lee (Antony.Lee) * Date: 2016-01-30 05:18
subprocess.__doc__ currently contains copies for the docstrings of a bunch of functions in the module (... but not subprocess.run).  The docs for the Popen class should be moved into that class' docstring.  The module's docstring also mentions the list2cmdline "method" (actually a function, and the qualifier "method"/"function" is missing the second time this function is mentioned ("The list2cmdline is designed ...")), but that function doesn't appear in `__all__` and thus not in the official HTML docs (yes, I know pydoc subprocess.list2cmdline works).
msg259241 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-01-30 07:02
IMO the doc strings should be reduced down so that it is a concise summary, and divided or merged into doc strings of each class, method, function. Less important details should be moved to the main RST documentation (if they aren’t already there).

Regarding list2cmdline(), see the comment next to __all__ in the source code, and Issue 11827. BTW modifying __all__ would not automatically affect what is written in the RST files, which is where the “official” HTML docs come from :)
msg259717 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2016-02-06 03:13
I agree. The module docstring should briefly describe the module and maybe list the contents, a line for each.  The itertools docstring does the latter; the math docstring does not, though I would not mind if it did.
msg278840 - (view) Author: Tim Mitchell (tim.mitchell) Date: 2016-10-18 03:58
Have stripped down the module __doc__ to a list of contents.
I chose to indent the descriptions of each argument to Popen. I know this is non-standard but it is such a long ramble otherwise.

Changed true -> :const:`True` in subprocess.rst.
msg278841 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2016-10-18 05:19
Thanks for the patch, Tim.

> Changed true -> :const:`True` in subprocess.rst.

This is out of scope for this issue and I actually prefer the current form.

Having method and class signatures in subprocess.__doc__ would make it less maintainable. I'd prefer having a short docstring that describes what the modules does, and what its modern API look like.

I don't think we should duplicate documentation of CalledProcessError, TimeoutExpired and Popen in their docstrings. Perhaps it would be better to just document which parameters are POSIX only.

Also, did you use the GitHub mirror to create the patch? If so, please use the official Mercurial repository: https://docs.python.org/devguide/setup.html#checkout Rietveld doesn't like patches created from the git repository so it was a bit hard to review the patch without using Rietveld. Thanks!
msg278924 - (view) Author: Tim Mitchell (tim.mitchell) Date: 2016-10-18 19:26
hg patch with changes forthcoming
msg278942 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-10-18 21:45
Thanks for tackling this one Tim. I agree with Berker that the :const:`True` changes are out of scope (some introduce errors and inaccuracies).

 class CalledProcessError(SubprocessError):
-    """Raised when a check_call() or check_output() process returns non-zero.
+    """Raised when a process run by check_call() or check_output() process
+    returns a non-zero exit status.

New text has stray “process” noun. The old text was good enough IMO.

+      output:      Output of the child process if it was captured by run() or
+                   check_output(). Otherwise, None.
     check_output() will also store the output in the output attribute.

I think that last paragraph is redundant and strange now that you already described the “output” attribute.

 class TimeoutExpired(SubprocessError):
+    Attributes:

The “cmd” attribute is missing from the list.

 class Popen(object):

Your patch seems to be based on 3.6 or 3.7, but you have omitted the new “encoding” etc parameters from the signature (and list of constructor parameters). What about just relying on the automatic signature for __init__()?

+    """ [. . .]
+      universal_newlines:
+        If universal_newlines is True, the file objects stdout and stderr are
+        opened as a text file, but lines may be terminated by any of '\n',

“opened as text files” would read better I think.

The escape codes will be translated to literal newlines etc in the doc string. The best solution is probably to make it a raw string: r""". . .""".

This universal_newlines entry has lots of details about newline translation of stdout and stderr, but fails to mention the translation of stdin. And I think the details about the child decoding its input should be added to the RST documentation before we consider adding it to the doc string.
msg278957 - (view) Author: Tim Mitchell (tim.mitchell) Date: 2016-10-19 01:25
Am now working from tip of default in mercurial (Python 3.6).

* Removed all changes to subprocess.rst.

subprocess.__doc__
* Trimmed down even further - removed function signatures.
* added note to see Python docs for complete description.

Exception docs
* just list attributes rather than describing them - they are pretty self-explanatory.

Popen.__doc__ 
* Trimmed down to just list arguments with 1 or 2 liner descriptions. 
* Added note to see Python docs for complete description.
* added encoding and errors arguments
msg279007 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-10-20 03:35
.
I left some comments on the code review.

Also, I’m not sure about the links to the online documentation. We don’t do this for other modules as far as I know. The pydoc module and help() commands already add their own links, which can be configured via PYTHONDOCS.
msg279075 - (view) Author: Tim Mitchell (tim.mitchell) Date: 2016-10-20 20:01
Changes as per Martins review.
msg279092 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-10-20 23:21
V3 looks good to me
msg279174 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-10-22 00:55
Here are corresponding patches for 3.5 and 2.7.
msg279476 - (view) Author: Roundup Robot (python-dev) Date: 2016-10-26 01:17
New changeset 720865fa61a4 by Martin Panter in branch '3.5':
Issue #26240: Clean up the subprocess module doc string
https://hg.python.org/cpython/rev/720865fa61a4

New changeset 8358c68579e9 by Martin Panter in branch '3.6':
Issue #26240: Merge subprocess doc string from 3.5 into 3.6
https://hg.python.org/cpython/rev/8358c68579e9

New changeset 0dd8b3f133f9 by Martin Panter in branch 'default':
Issue #26240: Merge subprocess doc string from 3.6
https://hg.python.org/cpython/rev/0dd8b3f133f9

New changeset 5a1edf5701f1 by Martin Panter in branch '2.7':
Issue #26240: Clean up the subprocess module doc string
https://hg.python.org/cpython/rev/5a1edf5701f1
History
Date User Action Args
2016-10-26 04:55:26martin.pantersetstatus: open -> closed
resolution: fixed
stage: commit review -> resolved
2016-10-26 01:17:27python-devsetnosy: + python-dev
messages: + msg279476
2016-10-22 00:55:59martin.pantersetfiles: + subprocess3-2.7.patch
2016-10-22 00:55:44martin.pantersetfiles: + subprocess3-3.5.patch

messages: + msg279174
stage: patch review -> commit review
2016-10-20 23:21:57martin.pantersetmessages: + msg279092
2016-10-20 20:01:39tim.mitchellsetfiles: + subprocess3.patch

messages: + msg279075
2016-10-20 03:35:02martin.pantersetmessages: + msg279007
2016-10-19 01:25:22tim.mitchellsetfiles: + subprocess2.patch

messages: + msg278957
2016-10-18 21:45:50martin.pantersetmessages: + msg278942
2016-10-18 19:26:42tim.mitchellsetmessages: + msg278924
2016-10-18 05:25:36Antony.Leesetnosy: - Antony.Lee
2016-10-18 05:20:00berker.peksagsetmessages: + msg278841
2016-10-18 04:59:55berker.peksagsetnosy: + berker.peksag
stage: needs patch -> patch review

versions: + Python 3.7
2016-10-18 03:58:21tim.mitchellsetfiles: + subprocess-docs.patch

nosy: + tim.mitchell
messages: + msg278840

keywords: + patch
2016-10-03 21:51:04Mariattasetnosy: + Mariatta
2016-09-12 22:56:58rbcollinssetnosy: + rbcollins
2016-03-18 16:18:18ezio.melottisetkeywords: + easy
nosy: + ezio.melotti
type: enhancement
2016-02-06 03:13:56terry.reedysetnosy: + terry.reedy
messages: + msg259717
2016-01-30 07:02:34martin.pantersetversions: + Python 2.7, Python 3.6
nosy: + martin.panter

messages: + msg259241

stage: needs patch
2016-01-30 05:18:03Antony.Leecreate