classification
Title: Add __all__ list to the string module
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.6, Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: SilentGhost, abarry, georg.brandl, leewz, martin.panter, python-dev, rhettinger, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2016-04-20 11:36 by leewz, last changed 2016-06-04 19:42 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
string___all__.patch abarry, 2016-04-20 13:22 review
string___all___2.patch abarry, 2016-04-20 13:33 review
ChainMap___all__.patch abarry, 2016-04-20 16:46 review
ChainMap___all___2.patch abarry, 2016-04-20 16:54 review
Messages (16)
msg263824 - (view) Author: Franklin? Lee (leewz) Date: 2016-04-20 11:36
I don't know if this kind of thing matters, but `from string import ChainMap` works (imports from `collections). It's used internally by `string`. This was done when ChainMap was made official:
https://github.com/python/cpython/commit/2886808d3809d69a6e9b360380080140b95df0b6#diff-4db7f78c8ac9907c7ad6231730d7900c

You can see in the above commit that `configparser` had `_ChainMap` changed to `ChainMap as _ChainMap`, but this was not done in `string`.
msg263825 - (view) Author: SilentGhost (SilentGhost) * (Python triager) Date: 2016-04-20 11:48
I wouldn't say it matter, "from configparser import _ChainMap" works too, but that's just how imports work in Python. There is not reason to do what you've done on purpose - ChainMap is undocumented and is clearly just a detail of implementation.

The fact that ChainMap pollutes namespace when doing star import ("from string import *"), however, is unfortunate. I believe there was an issue around that was supposed to add missing __all__, but I'm not able to find that issue atm.
msg263829 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-04-20 13:02
I agree that the namespace pollution is a valid bug. It affects “from string import *” and dir(string). Normally, modules either define __all__ listing their exported names, or they are really careful to define non-exported names beginning with underscores (_). Judging by the line “import re as _re”, somebody once tried to use the second technique for the “string” module.

Silent Ghost: perhaps you were thinking of Issue 23883 (adding to __all__ in various modules). This case is the opposite: we want to limit the exported names.
msg263831 - (view) Author: Franklin? Lee (leewz) Date: 2016-04-20 13:07
> The fact that ChainMap pollutes namespace when doing star import ("from string import *"), however, is unfortunate.

This is what I meant by "this kind of thing". (IPython also ignores underscored names for autocomplete suggestions, unless you start with an underscore. IPython doesn't respect `__all__` by default, but that's a report for IPython's tracker.)


> This case is the opposite: we want to limit the exported names.

(Well, not exactly the opposite. Same purpose.)
msg263832 - (view) Author: Franklin? Lee (leewz) Date: 2016-04-20 13:10
(Never mind that last bit. I see it now.)

> I agree that the namespace pollution is a valid bug. It affects “from string import *” and dir(string).

How do I get this behavior for `dir`? I get '_re' in `dir(string)`.
msg263833 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-04-20 13:14
Okay looks like I was mistaken about dir(). It ignores __all__, and just gives you everything. But the star-import point is still valid :)
msg263834 - (view) Author: Anilyka Barry (abarry) * (Python triager) Date: 2016-04-20 13:22
Patch fixes this the proper way by defining __all__ in the string module.
msg263835 - (view) Author: SilentGhost (SilentGhost) * (Python triager) Date: 2016-04-20 13:26
> Silent Ghost: perhaps you were thinking of Issue 23883 (adding to __all__ in various modules). This case is the opposite: we want to limit the exported names.

It was one of mine, issue 10895. Been dead for a while now.
msg263836 - (view) Author: Anilyka Barry (abarry) * (Python triager) Date: 2016-04-20 13:33
Thanks SilentGhost, seems my brain decided not to see uppercase names :) Attached patch adds Formatter and Template as well.
msg263850 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-04-20 16:37
> Judging by the line “import re as _re”, somebody once tried to use the second technique for the “string” module.

So I think it is preferable to use this technique for ChainMap.
msg263851 - (view) Author: Anilyka Barry (abarry) * (Python triager) Date: 2016-04-20 16:46
I added an underscore in front of ChainMap, but I kept the __all__ definition because I think it should be there regardless (like every module, basically).
msg263852 - (view) Author: Franklin? Lee (leewz) Date: 2016-04-20 16:46
Why not both?
msg263874 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-04-21 04:19
No strong opinion on changing the name to _ChainMap, as long as it only goes into 3.6.
msg263884 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2016-04-21 06:17
ChainMap___all___2.patch looks good.  I would be fine with this going into 3.5 as well (ChainMap is not part of the documented API for the string module).
msg264015 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-04-22 14:03
I’m not that fussed if the _ChainMap name is backported. I just thought it is safer to not do it; similar reasoning to why I only committed Issue 23883 patches to 3.6. I certainly think backporting __all__ is reasonable.
msg267302 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-06-04 19:42
New changeset 8136f9623d7f by Zachary Ware in branch '3.5':
Issue #26809: Add __all__ to string module.  Patch by Emanuel Barry
https://hg.python.org/cpython/rev/8136f9623d7f

New changeset 21ae58b77924 by Zachary Ware in branch 'default':
Closes #26809: Merge with 3.5
https://hg.python.org/cpython/rev/21ae58b77924
History
Date User Action Args
2016-06-04 19:42:17python-devsetstatus: open -> closed

nosy: + python-dev
messages: + msg267302

resolution: fixed
stage: patch review -> resolved
2016-06-04 18:54:35abarrysettitle: `string` exposes ChainMap from `collections` -> Add __all__ list to the string module
2016-04-26 06:13:37rhettingersetassignee: rhettinger ->
2016-04-22 14:03:14martin.pantersetmessages: + msg264015
2016-04-21 06:17:49rhettingersetmessages: + msg263884
2016-04-21 06:10:34rhettingersetassignee: rhettinger

nosy: + rhettinger
2016-04-21 04:19:58martin.pantersetmessages: + msg263874
stage: patch review
2016-04-20 16:54:38abarrysetfiles: + ChainMap___all___2.patch
2016-04-20 16:46:58leewzsetmessages: + msg263852
2016-04-20 16:46:44abarrysetfiles: + ChainMap___all__.patch

messages: + msg263851
2016-04-20 16:37:43serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg263850
2016-04-20 13:33:42abarrysetfiles: + string___all___2.patch

messages: + msg263836
2016-04-20 13:26:22SilentGhostsetmessages: + msg263835
2016-04-20 13:22:52abarrysetfiles: + string___all__.patch

nosy: + abarry
messages: + msg263834

keywords: + patch
2016-04-20 13:14:35martin.pantersetmessages: + msg263833
2016-04-20 13:10:46leewzsetmessages: + msg263832
2016-04-20 13:07:53leewzsetmessages: + msg263831
2016-04-20 13:02:34martin.pantersetnosy: + martin.panter
messages: + msg263829
2016-04-20 11:48:58SilentGhostsetnosy: + georg.brandl, SilentGhost
messages: + msg263825
2016-04-20 11:36:22leewzcreate