Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

subprocess __all__ is incomplete #55047

Closed
abadger mannequin opened this issue Jan 5, 2011 · 17 comments
Closed

subprocess __all__ is incomplete #55047

abadger mannequin opened this issue Jan 5, 2011 · 17 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@abadger
Copy link
Mannequin

abadger mannequin commented Jan 5, 2011

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
Copy link
Mannequin Author

abadger mannequin commented Jan 5, 2011

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 :-(

@abadger abadger mannequin added the stdlib Python modules in the Lib dir label Jan 5, 2011
@birkenfeld
Copy link
Member

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
Copy link
Mannequin Author

abadger mannequin commented Jan 5, 2011

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
Copy link
Mannequin Author

abadger mannequin commented Jan 5, 2011

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
Copy link
Member

pitrou commented Jan 6, 2011

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

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

@terryjreedy
Copy link
Member

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
Copy link
Member

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
Copy link
Mannequin

elibendersky mannequin commented Apr 12, 2011

Issue bpo-11827 seems to be strongly related

@serhiy-storchaka
Copy link
Member

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
Copy link
Member

vadmium commented Apr 7, 2015

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
Copy link
Member

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
Copy link
Mannequin

python-dev mannequin commented Apr 7, 2015

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
Copy link
Member

gpshead commented Apr 7, 2015

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
Copy link
Mannequin

python-dev mannequin commented Apr 7, 2015

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
Copy link
Member

gpshead commented Apr 7, 2015

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

@gpshead gpshead closed this as completed Apr 7, 2015
@gpshead gpshead self-assigned this Apr 7, 2015
@gpshead gpshead added the type-bug An unexpected behavior, bug, or error label Apr 7, 2015
@python-dev
Copy link
Mannequin

python-dev mannequin commented Apr 16, 2016

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
Copy link
Member

vadmium commented Apr 16, 2016

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

@ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

8 participants