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

configparser.__all__ is incomplete #71293

Closed
Unit03 mannequin opened this issue May 24, 2016 · 8 comments
Closed

configparser.__all__ is incomplete #71293

Unit03 mannequin opened this issue May 24, 2016 · 8 comments
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@Unit03
Copy link
Mannequin

Unit03 mannequin commented May 24, 2016

BPO 27106
Nosy @ambv, @vadmium
Files
  • configparser_all.patch
  • configparser_all.v2.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-09-09.07:55:11.880>
    created_at = <Date 2016-05-24.20:28:25.085>
    labels = ['type-feature', 'library']
    title = 'configparser.__all__ is incomplete'
    updated_at = <Date 2016-09-09.07:55:11.879>
    user = 'https://bugs.python.org/Unit03'

    bugs.python.org fields:

    activity = <Date 2016-09-09.07:55:11.879>
    actor = 'martin.panter'
    assignee = 'none'
    closed = True
    closed_date = <Date 2016-09-09.07:55:11.880>
    closer = 'martin.panter'
    components = ['Library (Lib)']
    creation = <Date 2016-05-24.20:28:25.085>
    creator = 'Unit03'
    dependencies = []
    files = ['42968', '44393']
    hgrepos = []
    issue_num = 27106
    keywords = ['patch']
    message_count = 8.0
    messages = ['266266', '266330', '266391', '266421', '274411', '274454', '274536', '275271']
    nosy_count = 4.0
    nosy_names = ['lukasz.langa', 'python-dev', 'martin.panter', 'Unit03']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue27106'
    versions = ['Python 3.6']

    @Unit03
    Copy link
    Mannequin Author

    Unit03 mannequin commented May 24, 2016

    That's a child issue of bpo-23883, created to propose a patch fixing configparser module's __all__ list.

    @Unit03 Unit03 mannequin added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels May 24, 2016
    @vadmium
    Copy link
    Member

    vadmium commented May 25, 2016

    Looks good to me

    @ambv
    Copy link
    Contributor

    ambv commented May 25, 2016

    The reason we specifically omitted Error was two-fold:

    • the name "Error" is very generic and during a star-import might easily shadow some other class of the same name;
    • Error is only a base class for exceptions raised by configparser and as such isn't part of the public API. You can see the same behavior in concurrent.futures for example. However, now I noticed configparser.Error is listed in the documentation so the assertion that "it's not public API" is effectively incorrect.

    So I'm torn a little here. On the one hand, it's nice to add Error for completeness. On the other hand, is this change solving a real issue or just satisfying your inner librarian? The reason we have to ask ourselves this question is that this change bears a small risk of breaking user code that was working before. Take a look at this example:

    from wave import *
    from configparser import *
    
    cfg = ConfigParser()
    cfg.read('appconfig.ini')
    try:
      with Wave_read(cfg['samples']['sad_trombone']) as wav:
        n = wav.getnframes()
        frames = wav.readframes(n)
    except Error as e:
      print("Invalid sample:", e)
    except KeyError as e:
      print("Can't find {!r} in the config".format(str(e)))
    else:
      play_sound(frames)
    

    Sure, it's bad code but the point is: it was working before and had a decent error handling strategy. With the change in __all__, it will just crash because wave.Error was never caught.

    Is this likely to happen? I don't think so. Knowing my luck, will it happen to somebody? Yeah. So the question remains: why do we want Error in __all__ in the first place? Is it worth it?

    @vadmium
    Copy link
    Member

    vadmium commented May 26, 2016

    My personal opinion is to include all public APIs. Names that are omitted from __all__ may not come up in pydoc, and it is surprising when I use “import * ” in the interactive interpreter to play with a module and there is something missing.

    To mitigate the risk of breaking code, I have been maintaining a list of the modules affected at <https://docs.python.org/3.6/whatsnew/3.6.html#changes-in-the-python-api\>, which warns that extra symbols will be imported in 3.6.

    On the other hand, there are other cases where people wanted to exclude APIs from __all__; I pointed out two at <https://bugs.python.org/issue26632#msg266117\>.

    @Unit03
    Copy link
    Mannequin Author

    Unit03 mannequin commented Sep 5, 2016

    Łukasz, Martin - I'm not sure how to proceed here, of course both ways out are reasonable. I'd be happy to provide (however small) patch for either one (adding Error to __all__ or just adding test for __all__). :)

    My inner librarian would of course either push the Error to fully become part of public API or change its name to non-public _Error or something, but I'd much rather rely on you in this regard.

    @ambv
    Copy link
    Contributor

    ambv commented Sep 5, 2016

    I'm still not convinced this change is *useful*. The list of incompatibilities in Python 3.6 in whatsnew is going to be used retrospectively by people already affected by a production issue, googling for an explanation as to what went wrong.

    I like the idea of adding a unit test that clarifies the omission is deliberate at this point.

    @Unit03
    Copy link
    Mannequin Author

    Unit03 mannequin commented Sep 6, 2016

    That's completely fine for me. I'm attaching the patch that just adds test for __all__, then. :)

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 9, 2016

    New changeset 739528288996 by Martin Panter in branch 'default':
    Issue bpo-27106: Add test for configparser.__all__
    https://hg.python.org/cpython/rev/739528288996

    @vadmium vadmium closed this as completed Sep 9, 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-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants