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

Module overrides member #222

Closed
patrick91 opened this issue Oct 25, 2023 · 3 comments
Closed

Module overrides member #222

patrick91 opened this issue Oct 25, 2023 · 3 comments

Comments

@patrick91
Copy link

Reproduction here: https://github.com/patrick-bug-repro/griffe-member-module

In my case I have a module with the same name as one of the function in a __init__.py file, it's a bit odd, but it works in python, unfortunately it looks like griffe is overriding it.

 .
├──  README.md
└──  strawberry
    ├──  __init__.py  # here there's a type function
    └──  type
        └──  __init__.py

output:

{
  "strawberry": {
    "kind": "module",
    "name": "strawberry",
    "labels": [],
    "members": [
      {
        "kind": "module",
        "name": "type",
        "labels": [],
        "members": [
          {
            "kind": "function",
            "name": "hello",
            "lineno": 1,
            "endlineno": 2,
            "labels": [],
            "members": [],
            "decorators": [],
            "parameters": [],
            "returns": null
          }
        ],
        "filepath": "strawberry/type/__init__.py"
      }
    ],
    "filepath": "strawberry/__init__.py"
  }
}

I'd expect to see the type function inside the members there too :)

@patrick91
Copy link
Author

This seems to be handled here:

griffe/src/griffe/mixins.py

Lines 186 to 208 in 039a702

if not member.is_alias:
# When reassigning a module to an existing one,
# try to merge them as one regular and one stubs module
# (implicit support for .pyi modules).
if member.is_module and not (member.is_namespace_package or member.is_namespace_subpackage):
with suppress(AliasResolutionError, CyclicAliasError):
if value.is_module and value.filepath != member.filepath:
with suppress(ValueError):
value = merge_stubs(member, value) # type: ignore[arg-type]
for alias in member.aliases.values():
with suppress(CyclicAliasError):
alias.target = value
self.members[name] = value # type: ignore[attr-defined]
if self.is_collection: # type: ignore[attr-defined]
value._modules_collection = self # type: ignore[union-attr]
else:
value.parent = self # type: ignore[assignment]
else:
self.members[parts[0]].set_member(parts[1:], value) # type: ignore[attr-defined]
class ObjectAliasMixin:
"""A mixin for methods that appear both in objects and aliases, unchanged."""

I'm not 100% familiar with the codebase, but I wonder if members should be come a dict[tuple[str, kind], MemberType] so you can have members of different kinds in the dictionary?

@pawamoy
Copy link
Member

pawamoy commented Oct 25, 2023

Thanks for the repro!

Yes, that's a known issue. It's actually a design flaw. When I designed Griffe, I centered it around the syntax to access objects: loader.modules_collection["package.module.submodule.Class.NestedClass.attribute"]. I failed to account for the fact that a module can have both a submodule and a member (imported or not) of the same name.

Instead of using (name, kind) tuples, the solution I envision is to split submodules out of the members dictionary, and update the syntax to support colons: package.module.submodule to access the submodule or the member of the same name when there's no such submodule (no ambiguity), and package.module:submodule to force access to the member and not the submodule of the same name when it exists.

As you can guess, that's quite a refactor, as there are a lot of places in the code that will need an update, so I kinda focused my energy elsewhere in the meantime 😅 But this issue is definitely in the 1.0 milestone (which exists only in my head): I will not release v1 before it is fixed.

Now if someone wants to take a stab at it, I'd gladly review a PR. The spec is simple (new submodules attribute in the Object class, support : in keys in the member mixins). But I think the implementation will have some traps and surprises 🙂

@pawamoy
Copy link
Member

pawamoy commented Mar 6, 2024

I'll close as duplicate of #124.

@pawamoy pawamoy closed this as completed Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants