Skip to content

bpo-30420: Document the cwd argument to check_output and check_call#1685

Merged
ncoghlan merged 2 commits into
masterfrom
alex-patch-1
May 26, 2017
Merged

bpo-30420: Document the cwd argument to check_output and check_call#1685
ncoghlan merged 2 commits into
masterfrom
alex-patch-1

Conversation

@alex

@alex alex commented May 20, 2017

Copy link
Copy Markdown
Member

No description provided.

@mention-bot

Copy link
Copy Markdown

@alex, thanks for your PR! By analyzing the history of the files in this pull request, we identified @birkenfeld, @gpshead and @ncoghlan to be potential reviewers.

@vadmium

vadmium commented May 20, 2017

Copy link
Copy Markdown
Member

There are other parameters missing compared to Popen, which is already mentioned in the documentation. Perhaps it would be better to add an ellipsis (...) or **params at the end of the signature to make this more obvious.

@alex

alex commented May 20, 2017

Copy link
Copy Markdown
Member Author

It's possible there are larger-scale changes that would be preferable. This was inspired by me using this parameter in some code, and @reaperhulk being skeptical that the parameter existed because it wasn't documented. This PR solves that problem, not broader challenges.

@ncoghlan

ncoghlan commented May 21, 2017

Copy link
Copy Markdown
Contributor

+1 for adding cwd to the "Frequently Used Arguments" section: https://docs.python.org/3/library/subprocess.html#frequently-used-arguments

In addition to check_call and check_output, it should also be listed in the signatures for call and run.

Regarding the general concern, I started writing a longer reply here, and then realised I would be better off just filing an issue and posting the relevant details there: http://bugs.python.org/issue30420

Basically, I think we have pretty solid (albeit anecdotal) evidence that our current approach to making these docs less overwhelming is genuinely misleading people as to the available API options, and we need to tweak things accordingly.

@ncoghlan ncoghlan changed the title Document the cwd argument to check_output and check_call bpo-30420: Document the cwd argument to check_output and check_call May 21, 2017
@ncoghlan ncoghlan removed the trivial label May 21, 2017
@alex

alex commented May 25, 2017

Copy link
Copy Markdown
Member Author

@ncoghlan good call (pun intended) on call and run

@ncoghlan ncoghlan merged commit 368cf1d into master May 26, 2017
@alex alex deleted the alex-patch-1 branch May 26, 2017 02:29
@ncoghlan

Copy link
Copy Markdown
Contributor

This at least makes it clearer that cwd is accepted as an argument by the convenience APIs, so I went ahead and merged it. However, I'll move the issue back to patch needed, as while this is an improvement, I don't think it fully resolves the underlying problem.

@alex

alex commented May 26, 2017

Copy link
Copy Markdown
Member Author

Sounds good.

Mariatta pushed a commit to Mariatta/cpython that referenced this pull request Jun 17, 2017
…ythonGH-1685)

Partially clarify the subprocess convenience API documentation by
explicitly listing the `cwd` parameter in their abbreviated signatures.

While this has been merged as an improvement, it doesn't fully
resolve the issue, as the `cwd` should also be covered in the
"Frequently Used Arguments" section, and the fact these APIs
pass unlisted keyword arguments down to the lower level APIs
is currently still unclear.
(cherry picked from commit 368cf1d)
@bedevere-bot

Copy link
Copy Markdown

GH-2253 is a backport of this pull request to the 3.6 branch

Mariatta added a commit that referenced this pull request Jun 20, 2017
…H-1685) (GH-2253)

Partially clarify the subprocess convenience API documentation by
explicitly listing the `cwd` parameter in their abbreviated signatures.

While this has been merged as an improvement, it doesn't fully
resolve the issue, as the `cwd` should also be covered in the
"Frequently Used Arguments" section, and the fact these APIs
pass unlisted keyword arguments down to the lower level APIs
is currently still unclear.
(cherry picked from commit 368cf1d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants