-
-
Notifications
You must be signed in to change notification settings - Fork 30.9k
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
Comments
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. |
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. |
Mmmh, what is the point of forcing people to adapt their module-like objects so that they have a __loader__ entry? |
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. |
+1 |
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. |
New changeset e39a8f8ceb9f by Brett Cannon in branch 'default': |
Some buildbots are failing after this: 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 |
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. |
New changeset bb023c3426bc by Brett Cannon in branch 'default': |
New changeset 97b7bd87c44c by Brett Cannon in branch 'default': |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: