Title: subprocess docs should emphasise convenience functions
Type: Stage: resolved
Components: Documentation Versions: Python 3.2, Python 3.3, Python 2.7
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: ncoghlan Nosy List: docs@python, eric.araujo, ezio.melotti, flox, ncoghlan, python-dev, rhettinger
Priority: normal Keywords:

Created on 2011-10-21 05:25 by ncoghlan, last changed 2011-11-11 12:24 by ncoghlan. This issue is now closed.

Messages (20)
msg146059 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2011-10-21 05:25
Many typical subprocess use cases can now be handled simply via the convenience functions:

However, readers of the documentation could be forgiven for not realising that, since the docs dive right in with Popen() and will scare most readers away in search of more user friendly APIs (or even other languages).

The module documentation should be reordered to introduce the helper function first, then Popen afterwards.

The "subprocess replacements" [1] section would ideally help address this, but it hasn't been updated to use the convenience function, instead using confusing direct calls to Popen.

msg146068 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2011-10-21 07:27
msg146095 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2011-10-21 15:23
From a quick look it seems to me that only 
-   output = Popen(["mycmd", "myarg"], stdout=PIPE).communicate()[0]
+   output = check_output(["mycmd", "myarg"])

can be changed.
Most of the other example access attributes of the Popen objects, such as stdin/stdout. is already used in a few places.

Another thing that I wanted to fix for a while is the syntax highlight in these examples.  Using '# becomes' instead of '==>' solves the problem for most of the examples, except for the first two that use `...`.  Using two separate blocks is another option.  A table with two columns (before/after) might also work, but it's difficult to get it wide enough to contain both the examples side by side.

About moving the convenience functions first, I'm not sure it works too well.
"This module defines one class called Popen:"
could be replaced by
"This module defines a class called Popen, and the convenience functions call(), check_call(), and check_output():"
where the three functions and possibly 'convenience functions' link to to the part of the page where they are defined.
msg146149 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2011-10-21 23:07
You couldn't just move them - you'd need to change the wording of how they cross-link to each other, since the explanations of the convenience function currently assume you understand how Popen works. I'd like us to get to the point where you only need to understand the whole Popen swiss army knife if you actually want that level of flexibility.
msg146151 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2011-10-21 23:18
Yep, what I was suggesting was an easier approach, but if someone is willing to reorganize the document, I agree that is better to start with an example that shows the most common use cases with the convenience functions and then gradually explain the convenience functions used in the example before moving to Popen and its intricacies.
The unittest doc for example follows the same approach, example first and then explanation.
msg146297 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2011-10-24 12:23
New changeset 2184df0e0f89 by Nick Coghlan in branch '2.7':
Issue #13237: Rearrange subprocess module documentation to emphasise the convenience functions and commonly needed arguments
msg146300 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2011-10-24 12:54
I'm reasonably happy with the changes I just checked in, but rather than doing multiple forward ports, my plan is to let them settle for a while, update them based on any feedback I get, then incorporate the final version into the 3.x series.
msg146429 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2011-10-26 11:06
New changeset 0b4df6701c4d by Nick Coghlan in branch '2.7':
Issue #13237: further updates to subprocess documentation
msg146430 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2011-10-26 11:16
New changeset f445c125aca3 by Nick Coghlan in branch '2.7':
Issue #13237: remove some details that only apply to the 3.x version of this module and cross reference the relocated warning about the dangers of invoking the shell with untrusted input
msg146431 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2011-10-26 11:19
Absent any further feedback, I think I'm done with the changes to the 2.7 subprocess docs. I'll let them sit for a few days, then do the forward port to 3.2 and default.

There are a couple of additional changes I'll add to the 3.x versions:
- mention redirecting to DEVNULL as a way of suppressing output when using call() and check_call()
- mention the UTF-8 assumption involved in using the universal newlines mode for pipes in 3.x (i.e. the stuff I mistakenly checked in to 2.7 and then reverted)
msg146432 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2011-10-26 11:34
New changeset 5dfe6d7f7c61 by Nick Coghlan in branch '2.7':
Issue #13237: fix typo
msg146441 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-10-26 16:13
I want to review the doc, but lack time right now.  For example, one function signature in your patch uses keyword-only arguments but 2.7 doesn’t support them.
msg146454 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2011-10-26 20:48
That's deliberate, as I'm only showing a selected subset of the full
signature at that point and using the subprocess API's with positional
arguments would lead to almost incomprehensible code. I'm not in any great
hurry to forward port though, so I'm happy to wait until you get a chance to
review the changes.
msg146472 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2011-10-27 03:51
I think Éric is referring to the foo(bar, *, baz=None) syntax.  In 2.7 you can drop the '*' and still leave only the keyword arguments that you think are more useful.
I also see that you converted a few examples to use shell=True, but afaiu that should be avoided (even if the input is trusted, it can be a bad example and lead to escaping issues).
For the 'exit 1' example you could add a note saying that the example is run with shell=True because 'exit' requires a shell, or just keep the more verbose Python version.
My concern is that many people just go trough the examples and copy/paste what they see without reading the text around unless it's necessary to make the command work, so, if many examples use shell=True, they might end up picking one of those.
msg146476 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2011-10-27 07:30
We can only protect people from themselves so much - "shell=True" is invaluable when you actually want to invoke the shell, and the shell has much better tools for process invocation and pipeline processing than Python does (since shells are, in effect, domain specific languages dedicated to those tasks).

If someone is blindly copying and pasting code from the internet, then shell injection attacks are likely to be the *least* of the security problems in anything they're building.

The point of the examples is to demonstrate the return code handling and using the shell is the easiest way to do that. I'll add a note to the docstrings to be aware of the security issues with the parameter, though.

As far as the keyword arguments go, no, I can't just drop the bare '*' from the abbreviated signature, because that would be making claims about the signature that are just plain *wrong* (there are other positional arguments that appear before 'stdin'). I'll add a note explaining that point somewhere in the 2.7 version, though.
msg146478 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2011-10-27 07:55
New changeset 2a2df6a72ccb by Nick Coghlan in branch '2.7':
Issue #13237: Make the subprocess convenience helper documentation self-contained aside from the shared parameter description. Downgrade the pipe warnings at that level to notes (since those pipes are hidden, people are unlikely to even try it)
msg146480 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2011-10-27 08:06
As the last checkin message says, I've made the documentation for the helper functions more self-contained. Each now has its own short "shell=True" warning with a pointer to the full explanation in the shared parameter description. There was also a copy-and-paste error in the Popen docs where the cross-reference for "shell=True" was a note rather than a warning, so I fixed that.

For the helper functions, people are unlikely to pass in "stdout=PIPE" or "stderr=PIPE" in the first place, since there's no point (you can't get at the Popen object, so you can't at the streams in order to read from them). Accordingly, I downgraded the relevant boxes to be notes rather than warnings (the boxes down near Popen.wait() and the data attribute descriptions remain warnings).

The latest version also uses the "keyword only" syntax for all three helper functions, but describes them with the following text in each case:

The arguments shown above are merely the most common ones, described below in :ref:`frequently-used-arguments` (hence the slightly odd notation in the abbreviated signature).

For 3.x, I'll reword that to:

The arguments shown above are merely the most common ones, described below in :ref:`frequently-used-arguments` (hence the use of keyword only argument notation in the abbreviated signature).
msg147285 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2011-11-08 12:12
New changeset e929d2a96d9b by Nick Coghlan in branch '3.2':
Issue #13237: Forward port subprocess module updates and explicitly document UTF-8 encoding assumption when universal_newlines=True

New changeset d3b159c43ee8 by Nick Coghlan in branch '3.2':
Issue #13237: Remove duplicate data value descriptions from the subprocess docs

New changeset 7545f4fb450c by Nick Coghlan in branch '3.2':
Issue #13237: Fix formatting error - the legacy shell commands weren't meant to be under the Notes heading

New changeset 0b50008bb953 by Nick Coghlan in branch 'default':
Issue #13237: Forward port from 3.2 of subprocess documentation updates. Needed quite a few adjustments to account for new features coming in 3.3
msg147286 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2011-11-08 12:14
Well, that forward port was definitely more complicated than I expected.

Additional reviews of both 3.2 and 3.3 (i.e. default) would be appreciated - there were quite a few adjustments needed to cope with changes between 2.7 and 3.2 and then additional changes between 3.2 and 3.3.
msg147430 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2011-11-11 12:24
Calling this one done - any further adjustments can be handled as new tracker issues.
Date User Action Args
2011-11-11 12:24:54ncoghlansetstatus: open -> closed
resolution: fixed
messages: + msg147430

stage: needs patch -> resolved
2011-11-08 12:14:34ncoghlansetmessages: + msg147286
2011-11-08 12:12:10python-devsetmessages: + msg147285
2011-10-27 08:06:27ncoghlansetmessages: + msg146480
2011-10-27 07:55:49python-devsetmessages: + msg146478
2011-10-27 07:30:56ncoghlansetmessages: + msg146476
2011-10-27 03:51:08ezio.melottisetmessages: + msg146472
2011-10-26 20:48:48ncoghlansetmessages: + msg146454
2011-10-26 16:13:10eric.araujosetmessages: + msg146441
2011-10-26 11:34:44python-devsetmessages: + msg146432
2011-10-26 11:19:02ncoghlansetmessages: + msg146431
2011-10-26 11:16:11python-devsetmessages: + msg146430
2011-10-26 11:06:23python-devsetmessages: + msg146429
2011-10-24 22:46:42ncoghlansetassignee: docs@python -> ncoghlan
2011-10-24 12:54:30ncoghlansetmessages: + msg146300
2011-10-24 12:23:18python-devsetnosy: + python-dev
messages: + msg146297
2011-10-23 02:30:19eric.araujosetnosy: + eric.araujo
2011-10-21 23:18:57ezio.melottisetmessages: + msg146151
2011-10-21 23:07:29ncoghlansetmessages: + msg146149
2011-10-21 15:23:01ezio.melottisettype: behavior ->
messages: + msg146095
2011-10-21 08:01:07floxsetnosy: + flox
type: behavior
2011-10-21 07:27:38rhettingersetnosy: + rhettinger
messages: + msg146068
2011-10-21 06:06:21ncoghlansettitle: subprocess docs should use emphasise convenience functions -> subprocess docs should emphasise convenience functions
2011-10-21 05:29:37ezio.melottisetnosy: + ezio.melotti
2011-10-21 05:25:01ncoghlancreate