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

Add __all__ list to the string module #70996

Closed
leewz mannequin opened this issue Apr 20, 2016 · 16 comments
Closed

Add __all__ list to the string module #70996

leewz mannequin opened this issue Apr 20, 2016 · 16 comments
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@leewz
Copy link
Mannequin

leewz mannequin commented Apr 20, 2016

BPO 26809
Nosy @birkenfeld, @rhettinger, @vadmium, @serhiy-storchaka, @leewz, @Vgr255
Files
  • string___all__.patch
  • string___all___2.patch
  • ChainMap___all__.patch
  • ChainMap___all___2.patch
  • 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 = None
    closed_at = <Date 2016-06-04.19:42:17.246>
    created_at = <Date 2016-04-20.11:36:22.932>
    labels = ['type-bug', 'library']
    title = 'Add __all__ list to the string module'
    updated_at = <Date 2016-06-04.19:42:17.244>
    user = 'https://github.com/leewz'

    bugs.python.org fields:

    activity = <Date 2016-06-04.19:42:17.244>
    actor = 'python-dev'
    assignee = 'none'
    closed = True
    closed_date = <Date 2016-06-04.19:42:17.246>
    closer = 'python-dev'
    components = ['Library (Lib)']
    creation = <Date 2016-04-20.11:36:22.932>
    creator = 'leewz'
    dependencies = []
    files = ['42536', '42537', '42540', '42541']
    hgrepos = []
    issue_num = 26809
    keywords = ['patch']
    message_count = 16.0
    messages = ['263824', '263825', '263829', '263831', '263832', '263833', '263834', '263835', '263836', '263850', '263851', '263852', '263874', '263884', '264015', '267302']
    nosy_count = 8.0
    nosy_names = ['georg.brandl', 'rhettinger', 'SilentGhost', 'python-dev', 'martin.panter', 'serhiy.storchaka', 'leewz', 'abarry']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue26809'
    versions = ['Python 3.5', 'Python 3.6']

    @leewz
    Copy link
    Mannequin Author

    leewz mannequin commented Apr 20, 2016

    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:
    2886808#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.

    @leewz leewz mannequin added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Apr 20, 2016
    @SilentGhost
    Copy link
    Mannequin

    SilentGhost mannequin commented Apr 20, 2016

    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.

    @vadmium
    Copy link
    Member

    vadmium commented Apr 20, 2016

    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 bpo-23883 (adding to __all__ in various modules). This case is the opposite: we want to limit the exported names.

    @leewz
    Copy link
    Mannequin Author

    leewz mannequin commented Apr 20, 2016

    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.)

    @leewz
    Copy link
    Mannequin Author

    leewz mannequin commented Apr 20, 2016

    (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).

    @vadmium
    Copy link
    Member

    vadmium commented Apr 20, 2016

    Okay looks like I was mistaken about dir(). It ignores __all__, and just gives you everything. But the star-import point is still valid :)

    @Vgr255
    Copy link
    Mannequin

    Vgr255 mannequin commented Apr 20, 2016

    Patch fixes this the proper way by defining __all__ in the string module.

    @SilentGhost
    Copy link
    Mannequin

    SilentGhost mannequin commented Apr 20, 2016

    Silent Ghost: perhaps you were thinking of bpo-23883 (adding to __all__ in various modules). This case is the opposite: we want to limit the exported names.

    It was one of mine, bpo-10895. Been dead for a while now.

    @Vgr255
    Copy link
    Mannequin

    Vgr255 mannequin commented Apr 20, 2016

    Thanks SilentGhost, seems my brain decided not to see uppercase names :) Attached patch adds Formatter and Template as well.

    @serhiy-storchaka
    Copy link
    Member

    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.

    @Vgr255
    Copy link
    Mannequin

    Vgr255 mannequin commented Apr 20, 2016

    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).

    @leewz
    Copy link
    Mannequin Author

    leewz mannequin commented Apr 20, 2016

    Why not both?

    @vadmium
    Copy link
    Member

    vadmium commented Apr 21, 2016

    No strong opinion on changing the name to _ChainMap, as long as it only goes into 3.6.

    @rhettinger rhettinger self-assigned this Apr 21, 2016
    @rhettinger
    Copy link
    Contributor

    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).

    @vadmium
    Copy link
    Member

    vadmium commented Apr 22, 2016

    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 bpo-23883 patches to 3.6. I certainly think backporting __all__ is reasonable.

    @rhettinger rhettinger removed their assignment Apr 26, 2016
    @Vgr255 Vgr255 mannequin changed the title string exposes ChainMap from collections Add __all__ list to the string module Jun 4, 2016
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 4, 2016

    New changeset 8136f9623d7f by Zachary Ware in branch '3.5':
    Issue bpo-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 bpo-26809: Merge with 3.5
    https://hg.python.org/cpython/rev/21ae58b77924

    @python-dev python-dev mannequin closed this as completed Jun 4, 2016
    @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

    3 participants