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

__loader__ = None should be fine #61317

Closed
brettcannon opened this issue Feb 3, 2013 · 11 comments
Closed

__loader__ = None should be fine #61317

brettcannon opened this issue Feb 3, 2013 · 11 comments
Assignees
Labels
type-bug An unexpected behavior, bug, or error

Comments

@brettcannon
Copy link
Member

BPO 17115
Nosy @brettcannon, @pitrou, @ezio-melotti, @ericsnowcurrently
Dependencies
  • bpo-17099: Raise ValueError when loader not defined for importlib.find_loader()
  • bpo-17117: Update importlib.util.module_for_loader/set_loader to set when loader = None
  • 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/brettcannon'
    closed_at = <Date 2013-05-04.21:30:05.612>
    created_at = <Date 2013-02-03.16:47:26.925>
    labels = ['type-bug']
    title = '__loader__ = None should be fine'
    updated_at = <Date 2013-05-04.21:37:17.214>
    user = 'https://github.com/brettcannon'

    bugs.python.org fields:

    activity = <Date 2013-05-04.21:37:17.214>
    actor = 'python-dev'
    assignee = 'brett.cannon'
    closed = True
    closed_date = <Date 2013-05-04.21:30:05.612>
    closer = 'brett.cannon'
    components = []
    creation = <Date 2013-02-03.16:47:26.925>
    creator = 'brett.cannon'
    dependencies = ['17099', '17117']
    files = []
    hgrepos = []
    issue_num = 17115
    keywords = []
    message_count = 11.0
    messages = ['181279', '181283', '181293', '181301', '181359', '181360', '188381', '188389', '188396', '188410', '188411']
    nosy_count = 6.0
    nosy_names = ['brett.cannon', 'pitrou', 'ezio.melotti', 'python-dev', 'eric.snow', 'gkcn']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue17115'
    versions = ['Python 3.4']

    @brettcannon
    Copy link
    Member Author

    There is no reason __loader__ can't be set to None like __package__. That means having xml.parsers.expat.(model|errors) set the attribute to None by default (and thus removing the exemption for those modules from test_importlib.test_api.StartupTests), updating the decorators in importlib.util to set __loader__ when it is None, and make importlib.find_loader() treat None the same as if the attribute isn't set (e.g. unifying the raising of ValueError in both cases).

    This will bring __loader__ back in alignment with __package__.

    In all honesty I would like to tweak imp.new_module()/PyModule_Create() to set both __package__ and __loader__ to None by default, but I don't know how much code out there relies on the absence of these attributes and not on them being set to None (although fixing all of that code is simply transitioning from hasattr(module, '__loader__') to getattr(module, '__loader__', None)). Luckily PEP-302 was updated a couple versions back to state the loaders **must** set these attributes, so hopefully as time goes on this will be less of a worry.

    @brettcannon brettcannon self-assigned this Feb 3, 2013
    @brettcannon brettcannon added the type-bug An unexpected behavior, bug, or error label Feb 3, 2013
    @brettcannon
    Copy link
    Member Author

    Screw it, forget the "like" and make that a "will happen". A What's New entry telling people to update their code to use a getattr() with a None default value for transitioning should be enough (and a single line change for the few people who would care about this).

    This will also require an updating of the importlib docs, language reference, and PEP-302 to say the language reference and importlib docs now supersede the PEP. For both attributes it should be stated that a value of None means "I don't know what the values should be", but that loaders should continue to be required to set them.

    @pitrou
    Copy link
    Member

    pitrou commented Feb 3, 2013

    Mmmh, what is the point of forcing people to adapt their module-like objects so that they have a __loader__ entry?
    Couldn't importlib just interpret the absence of __loader__ as a None?

    @brettcannon
    Copy link
    Member Author

    It will interpret its absence as None, but I would rather get the very common case of actual module instances just setting None automatically. Not having it set when None is already an accepted default value for __package__ seems inconsistent; either the lack of attribute should signify that the value is unknown for both values, or having None should, but not both. I'm advocating for the latter as it has an easier compatibility path. Plus I'm not worrying about random objects people stash into sys.modules, just real modules created from imp.new_module()/PyModule_Create(); if you muck with sys.modules you are on your own.

    @ericsnowcurrently
    Copy link
    Member

    In all honesty I would like to tweak imp.new_module()/PyModule_Create()...

    +1

    @ericsnowcurrently
    Copy link
    Member

    [document that] the language reference and importlib docs now supersede the PEP

    Agreed. PEP-302 is even crustier now than it was a year ago and Barry's new import page in the language reference obviates the need for 302 as the de facto spec.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented May 4, 2013

    New changeset e39a8f8ceb9f by Brett Cannon in branch 'default':
    bpo-17115,17116: Have modules initialize the __package__ and __loader__
    http://hg.python.org/cpython/rev/e39a8f8ceb9f

    @ezio-melotti
    Copy link
    Member

    Some buildbots are failing after this:
    http://buildbot.python.org/all/builders/x86%20Ubuntu%20Shared%203.x/builds/7840
    http://buildbot.python.org/all/builders/AMD64%20FreeBSD%209.0%203.x/builds/4536

    FAIL: test_everyone_has___loader__ (test.test_importlib.test_api.StartupTests)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "/srv/buildbot/buildarea/3.x.bolen-ubuntu/build/Lib/test/test_importlib/test_api.py", line 201, in test_everyone_has___loader__
        '{!r} lacks a __loader__ attribute'.format(name))
    AssertionError: False is not true : 'test.pydoc_mod' lacks a __loader__ attribute

    @brettcannon
    Copy link
    Member Author

    It's because test_pydoc is actively deleting the __loader__ of a test module. I'll have a look to figure out why it's doing that.

    @brettcannon brettcannon reopened this May 4, 2013
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented May 4, 2013

    New changeset bb023c3426bc by Brett Cannon in branch 'default':
    bpo-17115: Remove what appears to be a useless chunk of code which broke
    http://hg.python.org/cpython/rev/bb023c3426bc

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented May 4, 2013

    New changeset 97b7bd87c44c by Brett Cannon in branch 'default':
    bpo-17115: I hate you MS for not supporting C99.
    http://hg.python.org/cpython/rev/97b7bd87c44c

    @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
    type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants