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

submodules overshadow imports of the same name #124

Closed
machow opened this issue Jan 12, 2023 · 7 comments · Fixed by #250
Closed

submodules overshadow imports of the same name #124

machow opened this issue Jan 12, 2023 · 7 comments · Fixed by #250
Labels
bug Something isn't working

Comments

@machow
Copy link
Contributor

machow commented Jan 12, 2023

If a module's __init__.py has a submodule, foo.py. Then importing a function of the same name from it, seems to keep the module name, rather than the function. This seems like a slight mismatch from dynamically importing, which keeps the function instead.

from griffe.loader import GriffeLoader

loader = GriffeLoader()
mod = loader.load_module("some_package")
mod.members

# output:
#  {'foo': <Module(PosixPath('some_package/foo.py'))>}

Example files

some_package/__init__.py

from foo import foo

some_package/foo.py

def foo(): pass
@pawamoy
Copy link
Member

pawamoy commented Jan 13, 2023

Yep, Griffe's data model has a flaw: it does not distinguish between a submodule named foo and a member (imported or not) with the same name. A refactor is needed, but I didn't have time yet 🙂

@machow
Copy link
Contributor Author

machow commented Jan 13, 2023

Any chance you could point to some of the places that would need to be tweaked? I don't mind taking a look, since griffe is making quartodoc's job a million times easier..!

@pawamoy
Copy link
Member

pawamoy commented Jan 13, 2023

Well, currently submodules are stored in the members attribute of each object. Instead of doing that, we should store them in a submodules attribute. But then there are probably a lot of places in the code that will need to be updated.

@pawamoy
Copy link
Member

pawamoy commented Jan 13, 2023

Also, if we support that, we'll need to (re)introduce the colon : to delimitate modules from other objects in objects paths:
pkg:foo for the imported foo attribute, pkg.foo for the foo submodule.

@machow
Copy link
Contributor Author

machow commented Mar 7, 2023

Sorry for the long delay--I can definitely see how it creates a bit of complexity. Since griffe ends up keeping the module--which allows grabbing functions by their canonical path--it feels like the impact of overshadowing is not too bad. (I'd still love to pick this up at some point)

@pawamoy pawamoy added the bug Something isn't working label Jul 14, 2023
@pawamoy
Copy link
Member

pawamoy commented Mar 6, 2024

@machow @patrick91 @NiklasRosenstein

I started working on this. And, to be honest, I'm very tempted to leave things as they are and mark this as "won't fix". Without going into details, Python itself can be very confusing:

pkg
├── b
│   ├── c.py
│   └── __init__.py
└── __init__.py
# pkg/b/__init__.py
c = 1
# pkg/b/c.py
d = 1
>>> from pkg import b
>>> b.c
1
>>> from pkg.b.c import d  # import pkg.b.c has the same effect
>>> b.c
<module 'pkg.b.c' from '/home/pawamoy/t/pkg/b/c.py'>

Boom, Python just shadowed the member too. Just because of that, I'm very much inclined to declare the practice of using the same name for both a member and a submodule a bad practice.

Now, even if we were to re-introduce colons in the path syntax (pkg.b.c for the submodule, pkg.b:c for the member), things can still get weird with aliases (imported) modules. Let's add a.py which imports the b module:

pkg
├── a.py
├── b
│   ├── c.py
│   └── __init__.py
└── __init__.py
# pkg/a.py
from pkg import b

Python itself refuses to "pass through b" when it is an alias:

>>> from pkg.a.b import c
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ModuleNotFoundError: No module named 'pkg.a.b'; 'pkg.a' is not a package

So, how would we write this path in Griffe?

pkg.a:b.c for the submodule c of the member/module b in module a, and pkg.a:b:c for the member c of the member/module b in module a? Suddenly the components after a colon can be modules too? Also, two colons??

Or should we stick with pkg.a.b.c and pkg.a.b:c? That would cause issues with dynamic analysis because we would rely on the colon to distinguish between what should be importable modules, and what should be "gettable" attributes, and we saw above that __import__("pkg.a.b") would fail with pkg.a is not a package.

On the implementation side, supporting colons in object paths bring a lot of complexity, while it's already quite complex.

To conclude, I'm -1 on supporting this, and +1 to document this limitation prominently, as well as taking a stance and saying this is bad practice anyway.

@pawamoy
Copy link
Member

pawamoy commented Mar 8, 2024

While writing docs, I've ran more experiments, and it turns out that shadowing via import is less bad. With the same file tree, but these contents:

# pkg/b/__init__.py
from pkg.b.c import  c
# pkg/b/c.py
def c(): ...

...we get this behavior:

>>> from pkg import b
>>> b.c
<function thing at 0x78ae45e3a0c0>
>>> from pkg.b.c import c
>>> b.c
<function thing at 0x78ae45e3a0c0>  # still the function and not the module
>>> import pkg.b.c
>>> pkg.b.c
<function thing at 0x78ae45e3a0c0>  # even like this

In that case I'd say we could at least give precedence to the imported object rather than the submodule, since the intent is to expose the object.

But even then I still find all that confusing: somehow importing makes things OK, but declaring can make the submodule shadow the object.

pawamoy added a commit that referenced this issue Mar 11, 2024
pawamoy added a commit that referenced this issue Mar 11, 2024
pawamoy added a commit that referenced this issue Mar 11, 2024
pawamoy added a commit that referenced this issue Mar 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants