| 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) *  |
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) * |
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) * |
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) * |
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) * |
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) * |
Date: 2016-10-20 23:21 |
| V3 looks good to me |
|
|
| msg279174 - (view) |
Author: Martin Panter (martin.panter) * |
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 |
|
|