subprocess __all__ is incomplete

[abadger]

BPO	10838
Nosy	@birkenfeld, @terryjreedy, @gpshead, @orsenthil, @pitrou, @merwok, @abadger, @bitdancer, @vadmium, @serhiy-storchaka

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/gpshead'
closed_at = <Date 2015-04-07.23:12:49.365>
created_at = <Date 2011-01-05.20:52:47.181>
labels = ['type-bug', 'library']
title = 'subprocess __all__ is incomplete'
updated_at = <Date 2016-04-16.14:36:43.848>
user = 'https://github.com/abadger'

bugs.python.org fields:

activity = <Date 2016-04-16.14:36:43.848>
actor = 'martin.panter'
assignee = 'gregory.p.smith'
closed = True
closed_date = <Date 2015-04-07.23:12:49.365>
closer = 'gregory.p.smith'
components = ['Library (Lib)']
creation = <Date 2011-01-05.20:52:47.181>
creator = 'a.badger'
dependencies = []
files = []
hgrepos = []
issue_num = 10838
keywords = []
message_count = 17.0
messages = ['125468', '125470', '125476', '125482', '125485', '125734', '127020', '133567', '240177', '240206', '240213', '240236', '240238', '240239', '240240', '263549', '263562']
nosy_count = 11.0
nosy_names = ['georg.brandl', 'terry.reedy', 'gregory.p.smith', 'orsenthil', 'pitrou', 'eric.araujo', 'a.badger', 'r.david.murray', 'python-dev', 'martin.panter', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue10838'
versions = ['Python 3.5']

[abadger]

I have a compatibility module for subprocess in python-2.7 for people who are stuck on python-2.4 (without check_call) and they got a traceback from trying to use compat.subprocess.list2cmdline().

In order to use the stdlib's subprocess if it's of a recent enough version, I check the version and import the symbols from there using from subprocess import * in the compat module. Unfortunately, one of the people is using list2cmdline() in their code and list2cmdline() is not in __all__. Comparing the output, there's a few things not in __all__ in both python-2.7 and in python-3.1:

(From python-2.7, but python-3.1 boils down to the same list):

>>> sorted([d for d in  dir (subprocess) if not d.startswith('_')])
['CalledProcessError', 'MAXFD', 'PIPE', 'Popen', 'STDOUT', 'call', 'check_call', 'check_output', 'errno', 'fcntl', 'gc', 'list2cmdline', 'mswindows', 'os', 'pickle', 'select', 'signal', 'sys', 'traceback', 'types']
>>> sorted(subprocess.__all__)
['CalledProcessError', 'PIPE', 'Popen', 'STDOUT', 'call', 'check_call', 'check_output']

So, MAXFD, list2cmdline, and mswindows seem to be left out.

These could either be made private (prepend with "_"), or added to __all__ to resolve this bug. (I note that searching for "subprocess.any of those three" leads to some hits so whether or not they're intended to be public, they are being used :-(

[birkenfeld]

IMO none of these three are meant to be public, and neither are they documented. (Although the docs make a reference to "the list2cmdline *method*", which should probably just be removed.)

I remember a thread on python-dev about public-API-ness. Did we really conclude that all non-underscored names must be public and therefore added to __all__?

[abadger]

IIRC, it was more along the lines of: all private names should be underscored. The difference being that we get to choose whether currently non-underscored names should get underscored, should be deprecated and then underscored, or should be made public, put into __all__, and properly documented.

I think there was general agreement that leaving them non-underscored but expecting people to treat them as private wasn't a good idea.

[abadger]

For other's reference, there were three threads in November2010 that touch on this:

:About removing argparse.__all__ or adding more methods to it:
http://mail.python.org/pipermail/python-dev/2010-November/105147.html

:Removing tk interface in pydoc:
http://mail.python.org/pipermail/python-dev/2010-November/105375.html

The most on topic thread is the one with Subject:
:[Python-Dev] Breaking undocumented API:
http://mail.python.org/pipermail/python-dev/2010-November/105392.html

People broke threading a few times so you might have to search on the subject.

And ick. The thread's more of a mess than I remembered. Reading what Guido wrote last it seems like:

All private names should be prepended with "_" . Imported modules are the exception to this -- they're private unless included in __all__. Reading between the lines I think it's also saying that not all public names need to be in __all__.

So to resolve this ticket:

1. Is this the actual consensus from the end of those threads?
2. Are the three names mentioned in this ticket public or private?
   3a) If private, initiate deprecation and create underscore versions of the variables.
   3b) If public, documentation and adding to __all__ are good but not necessary.

[pitrou]

So, MAXFD, list2cmdline, and mswindows seem to be left out.

IMO they should all be prefixed with an underscore. Greg?

[terryjreedy]

My understanding is much like Toshio's: ambiguous (typically, undocumented or omitted from __all__) non-underscored names should be resolved, with the three possible outcomes listed, on a case-by-case basis.

[orsenthil]

On Thu, Jan 06, 2011 at 12:15:26AM +0000, Antoine Pitrou wrote:

IMO they should all be prefixed with an underscore. Greg?

+1 to this suggestion. It would make it consistent with expectations.
But yeah, I also think that all public methods should be in __all__ is
not a guarantee.

[elibendersky]

Issue bpo-11827 seems to be strongly related

[serhiy-storchaka]

For now there are three non-underscored names in subprocess that are missed in __all__: SubprocessError, TimeoutExpired, and list2cmdline. SubprocessError and TimeoutExpired are documented.

[vadmium]

IMO the two documented exceptions should definitely be added to __all__.

Not so sure, but list2cmdline() should probably be left out, with a comment explaining the decision. It is not mentioned in the main documentation that I can find, but it is mentioned in the doc string of the “subprocess” module. If it is only meant to be an internal detail, it shouldn’t be mentioned by name. If it is an external API (which I doubt), it should be documented better and added to __all__.

[bitdancer]

I believe it is and should remain internal. I agree that the mention should be deleted from the docstring. We removed it from the docs a while back but I guess we forgot the docstring.

(If there were an external API for doing what list2cmdline does, it would belong in the windows equivalent of the shlex module, something that has been discussed (and I think there is an issue in the tracker) but no one has stepped forward to write it :)

[python-dev]

New changeset 10b0a8076be8 by Gregory P. Smith in branch 'default':
Addresses Issue bpo-10838: The subprocess now module includes
https://hg.python.org/cpython/rev/10b0a8076be8

[gpshead]

the things left to to before closing this are to rename mswindows and MAXFD as those shouldn't be exported... and to wait for the windows buildbots to tell me if i missed adding anything to the intentionally_excluded list in the unittest.

[python-dev]

New changeset 4c14afc3f931 by Gregory P. Smith in branch 'default':
bpo-10838: Rename the subprocess.mswindows internal global to _mswindows.
https://hg.python.org/cpython/rev/4c14afc3f931

[gpshead]

Done. MAXFD was already gone in 3.5 (yay).

[python-dev]

New changeset cb38866e4c13 by Martin Panter in branch '3.5':
Issue bpo-10838: Run test__all__() everywhere, even if poll() is not available
https://hg.python.org/cpython/rev/cb38866e4c13

[vadmium]

See bpo-26782 for a follow-up with Windows.