-
-
Notifications
You must be signed in to change notification settings - Fork 33.2k
gh-121190: A better error message from importlib.resources.files() when module spec is None
#138531
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
base: main
Are you sure you want to change the base?
Conversation
…es() when module spec is not available
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this! 👏
Lib/importlib/resources/_adapters.py
Outdated
|
|
||
| TYPE_CHECKING = False | ||
| if TYPE_CHECKING: | ||
| from ..machinery import ModuleSpec |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Python stdlib is not typed (that's what typeshed is for) so you'll likely be asked to revert these changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please revert all type annotation changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks updated!
Lib/importlib/resources/_common.py
Outdated
|
|
||
| spec = wrap_spec(package) | ||
| if package.__spec__ is None: | ||
| raise TypeError(f"Can't access resources on a module with no spec: {package}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder is the error can be a bit more specific. I think most Python users will not know what a "spec" is? (I don't :-) )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I proposed a new message. wdyt?
| Adapt a package spec to adapt the underlying loader. | ||
| """ | ||
|
|
||
| def __init__(self, spec, adapter=lambda spec: spec.loader): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was the default adapter removed? Isn't this a breaking change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the simplicity sake, because it seems not to be used anywhere.
This default value was defined since this mechanism was introduced in #24670 but it was not used even in that commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As it does not pertain to the issue that's addressed in this PR I'd recommend reverting this change anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, got it 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way, importlib.resources module is somewhat special as its development is mainly in importlib_resources package which is then backported here, see https://github.com/python/importlib_metadata/wiki/Development-Methodology
So I am not sure if this PR should be merged here or moved over to the importlib_resources repo (CC @jaraco)
| Adapt a package spec to adapt the underlying loader. | ||
| """ | ||
|
|
||
| def __init__(self, spec, adapter=lambda spec: spec.loader): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As it does not pertain to the issue that's addressed in this PR I'd recommend reverting this change anyway.
It can be merged here, but we prefer to merge changes in importlib_resources to make it easier to keep the code in sync. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Co-authored-by: Daniel Hollas <danekhollas@gmail.com>
|
Thank you, I created a PR to |
importlib.resources.files(package)raises an error whenpackage.__spec__isNonebecause the code below refers tospec.namebutspecisNone.cpython/Lib/importlib/resources/_common.py
Line 117 in f19f1d8
The error would be
and #121190 said it's not helpful.
This PR adds a None-check to handle such a case and raise an error with a specific message.
(This is from PyConTW 2025 sprint)