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

feat: Support inheritance #170

Merged
merged 3 commits into from
Jun 30, 2023
Merged

feat: Support inheritance #170

merged 3 commits into from
Jun 30, 2023

Conversation

pawamoy
Copy link
Member

@pawamoy pawamoy commented Jun 15, 2023

Closes #138
Closes #153
Unblocks work on #96, mkdocstrings/python#40, mkdocstrings/python#13
Related to mkdocstrings/mkdocstrings#459, mkdocstrings/mkdocstrings#157, mkdocstrings/mkdocstrings#536

Tagging people who are probably interested: @samsja, @tazend, @cpcloud, @famura, @machow, @gab832, @analog-cbarber, @jayqi, @chrieke, @tristanlatr
Feel free to tag others. Also, subscribe to this PR to get future notifications: I'll explain the changes in this PR later so that we can discuss them. Or unsubscribe to stop getting notifications 🙂


Explanation:

  • the inspector only picks up non-inherited members
  • the visitor does not change at all
  • inherited members are fetched on demand with Object.inherited_members
  • inherited members are only fetched for classes, not instances (attributes), as instances have a type that can be used to point to the corresponding class
  • to fetch inherited members, we resolve base classes (get the actual Classes representing them) and use C3 linearization to build the dict of inherited members according to Python's MRO (method resolution order)
  • if multiple packages are involved in the inheritance, only members from pre-loaded packages (available in each module's modules_collection) are fetched
  • this gives control to users, allowing to choose how deep in the class inheritance to go ("inherit from pydantic classes, but not further") by pre-loading packages/modules of their choice

mkdocstrings and the Python handler:

  • we will be able to add an option inherit_members: true/false, false by default
  • when false, the members attribute of objects will be used to filter members
  • when true, the all_members property of objects will be used to filter members

Todo:

  • tests!!! 😄
    • generic tests
    • MRO tests
    • dynamic class base tests
  • fuzzing on stdlib
  • fuzzing on popular packages
  • docs 🤓

src/griffe/agents/nodes.py Show resolved Hide resolved
src/griffe/agents/inspector.py Outdated Show resolved Hide resolved
src/griffe/c3linear.py Show resolved Hide resolved
src/griffe/dataclasses.py Show resolved Hide resolved
src/griffe/dataclasses.py Outdated Show resolved Hide resolved
src/griffe/dataclasses.py Show resolved Hide resolved
src/griffe/dataclasses.py Show resolved Hide resolved
src/griffe/dataclasses.py Outdated Show resolved Hide resolved
src/griffe/agents/inspector.py Outdated Show resolved Hide resolved
tests/test_inheritance.py Show resolved Hide resolved
src/griffe/c3linear.py Show resolved Hide resolved
@pawamoy pawamoy changed the base branch from master to main June 19, 2023 14:52
@pawamoy pawamoy force-pushed the support-inheritance branch 2 times, most recently from f4de5f5 to 0706e6e Compare June 19, 2023 18:07
@machow
Copy link
Contributor

machow commented Jun 20, 2023

This looks great! One quick thought on...

if multiple packages are involved in the inheritance, only members from pre-loaded packages

I wonder if it will be helpful downstream in the python handler to emit a warning, if someone uses inherit_members: true and is missing some pre-loaded packages. It makes sense to me that people would need to pre-load packages, though!

@pawamoy
Copy link
Member Author

pawamoy commented Jun 20, 2023

Good point! We already log a debug message when a base class cannot be resolved:

logger.debug(f"Base class {base_path} is not loaded, or not static, it cannot be resolved")

Do you think that's enough? I don't want to add anything about upstream projects (mkdocstrings, mkdocstrings-python) within Griffe, as it's not concerned by them.

@machow
Copy link
Contributor

machow commented Jun 21, 2023

Ah, yeah that seems great! I think we might be thinking of the same thing, but using upstream and downstream in different ways (I think of mkdocstrings as downstream of griffe 😅).

I was thinking it might be helpful to give a more specific message in mkdocs-python, like "You may not see documentation for all members, since class X is not pre-loaded."

But the griffe message already seems very reasonable

@pawamoy
Copy link
Member Author

pawamoy commented Jun 21, 2023

I think of mkdocstrings as downstream of griffe

You're right, I should have said downstream 🤔

I was thinking it might be helpful to give a more specific message in mkdocs-python, like "You may not see documentation for all members, since class X is not pre-loaded."

Thanks, I get it now. Unfortunately, once in mkdocstrings-python, I don't think there's a clean way to rework the message 😕 Lets go with the Griffe debug message for now, and see later if it can be improved (if it generates too much confusion within users) 🙂 I'll definitely add such a message in the docs though!

@pawamoy
Copy link
Member Author

pawamoy commented Jun 29, 2023

This needs a bit more work for proper integration within mkdocstrings' Python handler. I'll explain later.

The consumer API can be used
when navigating or modifying the tree
once it was built by Griffe and extensions.

The producer API must be used
when building the objects tree,
so by Griffe itself and by extensions.

Using the consumer API while building
is risky because the consumer API
does ~~magic~~ convenient things
and trigger too-early computation
of different things like inherited members.
@pawamoy pawamoy marked this pull request as ready for review June 29, 2023 22:14
@pawamoy
Copy link
Member Author

pawamoy commented Jun 29, 2023

Ready for review, if some of you would like to run a final pass 🙂
The docs are not perfect, but don't worry, usage through mkdocstrings will be documented in mkdocstrings-python's own docs, with actual examples.

@pawamoy pawamoy merged commit 050678c into main Jun 30, 2023
13 of 16 checks passed
This was referenced Jun 30, 2023
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

Successfully merging this pull request may close these issues.

Make inheritance of members from the parent class configurable when inspecting Inherited menber
3 participants