jbms / sphinx-immaterial

Adaptation of the popular mkdocs-material material design theme to the sphinx documentation system
https://jbms.github.io/sphinx-immaterial/
Other
200 stars 32 forks source link

Add `python_apigen` support for separately documenting inherited members #122

Closed mhostetter closed 2 years ago

mhostetter commented 2 years ago

I mentioned this previously in passing in #99, but I think it would be nice to allow for adding separate (duplicate) documentation for an inherited member. I have a unique reason why that's required for me.

autodoc supports this with the "inherited-members" option of autodoc_default_options. Providing this option (as True) will include all inherited members (except from object) in the class. And providing this option with a base class name will include all inherited members except those from that base class.

I'm proposing we either add a new python_apigen_inherited_members option (or something similar) or read autodoc_default_options to allow for duplicating inherited members. I believe we could utilize the existing code that documents inherited members whose parents are undocumented.

I tried looking through the code to see where I could add this. I didn't immediately find the right place. If you can provide me some pointers of where to look, I'm willing to try to implement this on my own. I also understand if it's easier to do something yourself than help someone else do it inferiorly.

jbms commented 2 years ago

Do you want this to just happen unconditionally (all inherited members of all classes are treated as direct members) or does it need to be configurable in some way, e.g. via regular expression matching the name?

jbms commented 2 years ago

Note that currently, if both classes A and B inherit a member x from an undocumented base class C, that member will not be duplicated --- it will be linked as a member from both but there will just be a single document created for it, which will show it with a parent class of either A (due to the sort order).

mhostetter commented 2 years ago

Do you want this to just happen unconditionally (all inherited members of all classes are treated as direct members) or does it need to be configurable in some way, e.g. via regular expression matching the name?

I think unconditionally. I use the autodoc-skip-member callback to assist in pruning which members I really want documented. (The issue for me was a class that inherits from np.ndarray, which has tons of members I don't want to document. So I set them to skip = True in that callback.)

I know at one point you said you don't use the autodoc configs, but some autodoc is running under the hood, correct? I noticed if I removed my app.connect("autodoc-skip-member", autodoc_skip_member) then I got totally different results. So it seems like that code is running.

mhostetter commented 2 years ago

This might be too much info, but the real problem I'm trying to solve is the following. I created a workaround to document "class properties" in Sphinx with code written in Python < 3.9. However that workaround no longer works with python-apigen due to the inherited members issue.

In Python 3.9 and greater, you can make a class property like this and Sphinx will document it correctly.

class A:
    @classmethod
    @property
    def foo(cls) -> str:
        return "bar"

However, before Python 3.9, you must make a class property like this, but Sphinx does not document it.

class AMeta(abc.ABCMeta):
    @property
    def foo(cls) -> str:
        return "bar"

class A(metaclass=AMeta):
    pass

Sphinx recently added support for Python 3.9 style class properties. I found I can monkey patch my Python 3.7 and Python 3.8 code to work with Sphinx by adding the following monkey patch to conf.py.

import my_module

def classproperty(obj):
    ret = classmethod(obj)
    ret.__doc__ = obj.__doc__
    return ret

class A(my_module.A):
    foo = classproperty(type(my_module.A).foo)

my_module.A = A

This works with autosummary and autodoc. What I found with python_apigen is that when I redefine my_module.A in conf.py, all of the other members defined in the "real" class A are not documented because I believe they're considered "inherited members".

I'm open to other workarounds or solutions. Getting Sphinx to document class properties in Python <3.9 has been the bane of my existence! And it took forever to reach the current workaround. This class property issue is currently holding me back from porting over to the new, sexy python-apigen 🔥. That's the backstory...

I can make a simple standalone project for testing, if you'd like.

2bndy5 commented 2 years ago

There's no such thing as "too much detail" if it helps us understand where you're coming from. Your backstory is much appreciated.

jbms commented 2 years ago

Does it work to instead just do:

my_module.A.foo = classproperty(type(my_module.A).foo)

jbms commented 2 years ago

I see that doesn't work, because the assignment is intercepted by the existing property on AMeta --- not sure if there is a way to assign a foo attribute on A after it is defined.

jbms commented 2 years ago

It looks like temporarily removing the property from AMeta allows you to assign the property to A:

import abc

class AMeta(abc.ABCMeta):
    @property
    def foo(cls) -> str:
        return "bar"

class A(metaclass=AMeta):
    pass

def classproperty(obj):
    ret = classmethod(obj)
    ret.__doc__ = obj.__doc__
    return ret

foo = AMeta.foo
del AMeta.foo
A.foo = classproperty(foo)
AMeta.foo = foo

I haven't tested this with Sphinx, but I'm guessing it will work, if Sphinx interpreted the original example as a classproperty.

jbms commented 2 years ago

Actually, though, I don't think the issue in your case with python-apigen was with the inherited members. I think it might instead have been that the "original" class my_module.A will have a canonical name of my_module.A, but the redefined my_module.A (which is a different class) is also reachable from the name my_module.A. I think the code currently doesn't handle that case well.

Note that if the original class had a different undocumented name, I think python-apigen would work correctly, since those inherited members would not be reachable from anywhere else, so they would be documented as if they were members of the derived class.

If you can create a test case for that issue and add it to add it to python_apigen_test that would be helpful.

mhostetter commented 2 years ago

@jbms thanks for all your responses. I was able to get it to work! I'm glad I provided the backstory.

Note that if the original class had a different undocumented name, I think python-apigen would work correctly, since those inherited members would not be reachable from anywhere else, so they would be documented as if they were members of the derived class.

I did think of that and tried it previously. The issue that I found was all my internal code would need to point to the undocumented class name (which is non ideal) or I'd have to monkey patch each module that imports that class (which is most modules). I found issues with isinstance() and issubclass() failing when they normally wouldn't. Perhaps I didn't do it correctly.

It looks like temporarily removing the property from AMeta allows you to assign the property to A:

This solution worked for me! I tried something like my_module.A.foo = classproperty(type(my_module.A).foo) before, but to no avail, like you observed. This deletion trick, however, worked.

It ended up being a little more complicated for me because I have two hierarchies of metaclasses, and apparently you have to delete the properties from each. But the monkey patching code is algorithmic and no big deal. I'm very happy with the monkey patch. Thanks for taking the time to help me resolve this issue!!! I'm very excited to finish porting my library over to python-apigen. 😃

What kind of unit test are you looking for?

Here's the code I ended up with, if anyone's interested. Not beautiful, but it works.

# conf.py
ArrayMeta_properties = ["name", "characteristic", "degree", "order", "irreducible_poly", "primitive_element", "dtypes", "display_mode", "ufunc_mode", "ufunc_modes", "default_ufunc_mode"]
FieldArrayMeta_properties = ["properties", "name", "characteristic", "degree", "order", "irreducible_poly", "is_primitive_poly", "primitive_element", "primitive_elements", "quadratic_residues", "quadratic_non_residues", "is_prime_field", "is_extension_field", "prime_subfield", "dtypes", "display_mode", "ufunc_mode", "ufunc_modes", "default_ufunc_mode"]
for p in FieldArrayMeta_properties:
    # Fetch the class properties from the private metaclasses
    if p in ArrayMeta_properties:
        ArrayMeta_property = getattr(galois._domains._array.ArrayMeta, p)
    FieldArrayMeta_property = getattr(galois._fields._array.FieldArrayMeta, p)

    # Temporarily delete the class properties from the private metaclasses
    if p in ArrayMeta_properties:
        delattr(galois._domains._array.ArrayMeta, p)
    delattr(galois._fields._array.FieldArrayMeta, p)

    # Add a Python 3.9 style class property to the public class
    setattr(galois.FieldArray, p, classproperty(FieldArrayMeta_property))

    # Add back the class properties to the private metaclasses
    if p in ArrayMeta_properties:
        setattr(galois._domains._array.ArrayMeta, p, ArrayMeta_property)
    setattr(galois._fields._array.FieldArrayMeta, p, FieldArrayMeta_property)
jbms commented 2 years ago

What I had in mind was putting the following in a test module.

class A:
  """My class"""
  def foo(self) -> int:
    """Does something."""

class _A(A):
  def bar(self) -> int:
    """Does something else."""

_A.__doc__ = A.__doc__
A = _A

Then checking that both foo and bar are listed as members.

Like the existing tests in python_apigen_test.py, it isn't necessary to actually build the documentation, you can just inspect the data.entries dict generated by the extension.

mhostetter commented 2 years ago

Yes, I'll submit a PR with the additional unit test. Again, I greatly appreciate the help.

domWalters commented 1 year ago

Hi,

I've just been trying to get similar functionality to this working for a project I'm working on to some success, but not fully.

I have a class Library which inherits a class NamedAsset which has a property name.

When I generate documentation, the "Properties" page for Library lists name, but clicking on it takes me to the NamedAsset documentation, not a replicated copy for Library.

Additionally, in the ToC on the left side, it only shows me the direct properties of Library and none of the inherited ones.

My config sets:

autodoc_default_options = {
    "members": True,
    "inherited-members": True,
    "show-inheritance": True,
    "special-members": True,
    "undoc-members": True,
}

Was there something else you had to do @mhostetter to get the "copied inheritance" that you described?

mhostetter commented 1 year ago

Was there something else you had to do @mhostetter to get the "copied inheritance" that you described?

I was never able to get the "copied inheritance" to work. I was able to resolve a related issue, with the team's help, though.

I do still think there is value in adding a python_apigen_inherited_members option of some kind. I think there are certain circumstances where enumerating all available methods and properties on one page is useful, rather than a user having to trace the entire inheritance tree.

domWalters commented 1 year ago

This is what my generated docs look like (I'm part way through a refactor, so excuse the incoherences).

sphinx_bug

I'd be somewhat happier if the ToC on the left also listed the inherited stuff (note how tests is the only listed property).

I don't overly mind that clicking it goes to the class it was inherited from (other than potential user confusion, as those other classes are not user facing).

jbms commented 1 year ago

If you exclude the non-user-facing classes from your documentation, then the inherited members will be documented as regular members.

We could probably use better ways to exclude classes from the documentation, but for now you can accomplish that by putting them in a separate module, using a name with a leading underscore, or possibly defining __all__ (I don't think we've tested that last one, but hopefully it works).

jbms commented 1 year ago

Independent of that solution, as for making the inherited members show up in the TOC, that basically amounts to having the same page at more than one location within the TOC, which is not something Sphinx particularly supports, although it also does not particularly prevent it. This needs some fixes to nav_adapt.py to work well, but could probably be made to work okay.

domWalters commented 1 year ago

So I've just tried moving the non-user-facing classes to have a leading underscore.

This has resulted in the definition of name now attaching to another of my public classes called Application (alphabetically first) that also inherits name.

If I click on Library.name I get taken to Application.name.

jbms commented 1 year ago

Yes, currently in that case one of the "paths" for reaching the entity is picked as the canonical path. There isn't support at the moment for duplicating an entity, but it could be added as an option. I think it is not entirely trivial to implement though.