-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Allow namespace packages in _EditableFinder #4954
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
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.
Thank you very much for working on this Jason.
I find that the namespace handling is something very complicated.
if candidate_path.is_dir(): | ||
# treat it like a PEP 420 namespace package | ||
spec = ModuleSpec(fullname, None, is_package=True) | ||
spec.submodule_search_locations = [str(candidate_path)] | ||
return spec | ||
return None |
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.
Would something like the following make sense?
if candidate_path.is_dir(): | |
# treat it like a PEP 420 namespace package | |
spec = ModuleSpec(fullname, None, is_package=True) | |
spec.submodule_search_locations = [str(candidate_path)] | |
return spec | |
return None | |
return _EditableNamespaceFinder.find_spec(fullname) |
(or some kind of refactoring on _EditableNamespaceFinder
so that we reuse the same logic?)
Previously when I was working with namespaces, I found that sometimes the import machinery ignores the ModuleSpec
object returned by the finders and create a completely new one.
For some cases I found that I had to use a sys.path_hooks
instead of sys.meta_path
, that is why I implemented the namespace handling in _EditableNamespaceFinder
. But it seems that it was not enough 😅. I believe that the relevant portion of the code is
https://github.com/python/cpython/blob/580888927c8eafbf3d4c6be9144684117f0a96ca/Lib/importlib/_bootstrap_external.py#L1259-L1285.
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.
Maybe. I briefly considered using the _EditableNamespaceFinder, but it also felt like that finder had too much behavior that was maybe unneeded for these (ns-package-in-simple-package) packages. Because the parent simple package can't have multiple paths, that also means there's no possibility of this child ns package having multiple paths. In the example, test
has one home, so test.b
will only ever have one home.
Although, now that I think about it, since Setuptools is doing something special here, and constructing packages virtually from various sources, it's conceivable that test.b
could in fact have multiple paths. That is, test.b.foo
could come from one path while test.b.bar
or test.b.c
comes from another. I don't think that's possible given the current package_dir
implementation that test.b
could ever have modules in multiple paths, but it seems perfectly possible that package_dir = {'test.b.c': 'some path'}
could extend the paths and namespace of test.b
. And in that case, maybe reusing the functionality of _EditableNamespaceFinder makes sense. I also like the elegance of fewer lines of code. I'll give it a try.
Summary of changes
Closes
Pull Request Checklist
newsfragments/
.(See documentation for details)