From what I've seen on several c.l.p threads, some people have a tough
time figuring the correct 'args' argument to subprocess.Popen's
constructor. In an effort to cut down on such discussions in the future,
I've written the attached docs patch to better explain the subject.

I'm not an rst/Sphinx guru and thus was unable to actually test the
patch, so there might be some (hopefully minor) formatting errors.

We are trying to cut down on the number of "warning" directives in the
docs; I think a "note" directive would be appropriate for yours.

Ok, changed to "note" directives instead of "warning"s. Anything else
that keeps this from being applied?

This is a particularly verbose patch for the information it is trying to convey.

The note starts out "Do not (do x) unless you're not (doing y)". This is confusing. I believe this means "If you are (doing y), you may (do x)". If so, please write it so. If not, please write what you do mean.

After thinking about it, "Only (do x) if you are (doing y)" might be a better rewrite. Or "You may (do x) only if you are (doing y)"

Change "be mindful of escaping" to the more direct "escape". Perhaps give a list of possible special characters.

I agree that this seems a bit verbose for reference documentation. Perhaps there should be a separate PyWiki entry or HowTo doc. That could, for instance, list the special characters for various shells if they are not all the same.

PS. I only looked at the style of the patch. Once that is clearer, someone who knows more than me needs to review it for content correctness.

Working on a more concise new draft...

Okay; new, hopefully better, draft. Feedback?

The raw_input() doesn't provide anything. I'd just drop that and pass the string directly to shlex.split.

"Do not put an argument-taking option together with its argument as a single item in the *args* list"
-- Something like "Argument-taking options should not be grouped with their arguments in the *args* list." may be a cleaner read, and avoids the big scary "do not".

"Because that is incorrect"
-- Maybe something like "shlex.split would suggest the following" instead?

Gonna have to disagree about the raw_input(), because the escaping involved would complicate the example and could be distracting/confusing.
Rest of Brian's suggestions taken into account.

I still think this is much too verbose. The second note looks redundant with the first one and the third one.
Remember, the notes you are adding may be useful, but they'll also make reading the whole page more tedious.

Well, the same basic example is used for cohesiveness, but the issue/pitfall being highlighted in each note is distinct. But you have a point about shlex being pointed out twice, so here's a version with that redundancy excised.

I like the idea of pointing out that shlex can be used to determine exactly what to pass to subprocess, but I agree that the proposed patch is too wordy (and still too much in a negative voice).

Here is an alternate simpler patch.

Note that while I have also clarified the last sentence in the shell=True case, I think that this is in fact a bug in the Popen API. As far as I can tell the ability to pass arguments to the shell after the -c is useless, and I think Popen should instead raise a ValueError if passed a sequence when shell is True. But that is a different bug report....

Hmm. Somehow the patch got lost. Let's try again.

Woops, spotted a word I left out. Fixed.

From David's patch:

Note in particular that options and their arguments go in separate list
elements, while arguments that need quoting when used in the shell
(such as filenames containing spaces or the python command shown
above) are single list elements.

It's not always true that options and their arguments should be in separate elements. For example:
['/bin/ls', '-l', '--block-size=1024']
I understand the sentiment, but if we're trying to give guidance that people can follow without understanding the underlying issue, then they should be correct all the time.

Also, the code snippet includes subprocess without using it.

Counterpatch incorporating R. David Murray's succinctness improvements while retaining correct positioning of the first note, managing to incorporate the 3rd note not present in Mr. Murray's, and including more precise wording to address the problem Eric Smith pointed out.

My placement of the note was carefully considered. It is discussing the shell=False case and IMO belongs after that paragraph. I understand now your concern about the note I omitted...and again I think this is a bug in the Popen API. If shell=False, I think Popen should generate an error if args is a string. Absent that fix, I've reworded the existing sentence along the lines you suggest, but using positive language rather than negative. I've incorporated your fix for Eric's bug report, but I haven't added in the explicit references to the strings in the example. IMO those are obvious and don't need repeating, but if others think it is clearer to add them, I'm fine with that.

As for subprocess, IMO there's no need to show the subprocess call. The shlex result shows the list that would go into the subprocess call, and I think that's sufficient. What do others think?

I've updated my version of the patch.

By the way, I've been wanting the Popen docs improved for a long time but never got around to it, so thanks for pushing for this.

The following sentences look like a distraction to me:

« Additional quoting may be required because the entire string is a Python string. It may be useful to use a Python raw string in complex cases. »

It is true of every string literal and is not specific to the subprocess module.

Also, I don't understand what the reference to "-c" is about. subprocess isn't only for executing Python interpreters. Or perhaps you are talking about "sh -c", but it is quite cryptic.

I'm happy to delete the two sentences about quoting.

As for -c, you are so right that it is cryptic. In the new version of the patch I've changed the sentence to be as precise as possible, but I'm not at all convinced that it is less confusing. This is why I think the API should be changed.

This version takes Murray's most recent draft, applies some minor tweaks from my prior patch, and has the "python -c" etc. changed to "echo '$MONEY'" so the sh -c comment is completely unambiguous (and it's a simpler example generally).

Whether the subprocess.Popen() call should be demonstrated in the code snippet remains an open issue.

The change to echo is an excellent improvement. You forgot to change 'python' to 'echo' in the following paragraph, though. You are also correct about /bin/sh vs sh, my bad. And I was even looking at the source code when I wrote that...

Now you need to put the import of subprocess back in!

Otherwise it looks good to me.

Okay, now if this could just get dev review...

I think this is an improvement to the existing docs, and should be committed.

Committed for 2.7 as r77959.

Still needs to be merged to the other branches.

When merging to py3k, don't forget to modify the print statement to be a function.

Merged as part of r77961 (2.6), r77962 (py3k), and r77963 (3.1). Print fixed for py3.

Thanks to all for the copious feedback & suggestions, and R. David Murray for his superior docs writing skills!

One problem with the 3.x versions: the raw_input() should be input().

Thanks for catching that. Fixed r77987 r77988.

As recommended by eric.smith on bpo-7950, I'd like to suggest further extending the documentation to include a security warning about (quite easily) possible code injection bugs when using the shell=True parameter (similar to other places in the documentation).