jbms / sphinx-immaterial

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

Issue when default argument value is a class #140

Open mhostetter opened 2 years ago

mhostetter commented 2 years ago

I found this issue in my project and have reproduced it in this foo-example.zip module... When passing a class as a default argument, the parameter linking and type hint parsing blows up. Notice below that function_1() fails to render the docstring correctly if A is passed in, but function_2() will render correctly.

class Base:
    pass

class A(Base):
    pass

class B(Base):
    pass

def function_1(arg_1: int, arg_2: Type[Base] = A, arg_3: str = "something"):
    """
    This is the docstring for function_1.

    Parameters
    ----------
    arg_1
        The first argument.
    arg_2
        The second argument.
    arg_3
        The third argument.
    """
    pass

def function_2(arg_1: int, arg_2: Optional[Type[Base]] = None, arg_3: str = "something"):
    """
    This is the docstring for function_2.

    Parameters
    ----------
    arg_1
        The first argument.
    arg_2
        The second argument.
    arg_3
        The third argument.
    """
    pass

image

# Some warnings from the build.
foo.function_1:: WARNING: Parameter name 'arg_1' does not match any of the parameters defined in the signature: ['arg_1: int', "arg_2: ~typing.Type[~foo._bar.Base] = <class 'foo._bar.A'>", "arg_3: str = 'something'"]
foo.function_1:: WARNING: Parameter name 'arg_2' does not match any of the parameters defined in the signature: ['arg_1: int', "arg_2: ~typing.Type[~foo._bar.Base] = <class 'foo._bar.A'>", "arg_3: str = 'something'"]
foo.function_1:: WARNING: Parameter name 'arg_3' does not match any of the parameters defined in the signature: ['arg_1: int', "arg_2: ~typing.Type[~foo._bar.Base] = <class 'foo._bar.A'>", "arg_3: str = 'something'"]

image

jbms commented 2 years ago

Autodoc has to convert the function signature, which it extracts in various ways, back into text form, so that it can be fed into the Sphinx Python domain directive; the Python domain then attempts to parse the signature as a Python AST. If that fails it falls back to a simpler form of parsing (basically just splitting on commas) that doesn't allow cross linking to work.

I think this may be a bug in autodoc, as it is producing invalid Python syntax <class 'foo._bar.A'> instead of just e.g. foo._bar.A. Can you check if this issue occurs independent of this theme?

mhostetter commented 2 years ago

Ah, good catch!! I tested with a different theme and saw similar phenomena. I'll search the Sphinx issue list and open a new issue if this wasn't previously reported. If it's a simple fix, I wonder if monkey patching here is possible until a fix can get merged into Sphinx (seems to take a while).

image

image

jbms commented 2 years ago

Monkey patching is likely possible, but will depend on what the fix is --- probably the place to start looking is sphinx.util.inspect.stringify_signature

mhostetter commented 2 years ago

@jbms I did some digging and made this change https://github.com/mhostetter/sphinx/commit/6008405161957a6eafed4a9c53ed9fd3a15403a8.

The sig entering stringify_signature() is

(arg_1: int, arg_2: Type[foo._bar.Base] = <class 'foo._bar.A'>, arg_3: str = 'something')

and the return string is

(arg_1: int, arg_2: ~typing.Type[~foo._bar.Base] = ~foo._bar.A, arg_3: str = 'something')

However, during some downstream processing the ~foo._bar.A default is converted to / displayed as ~ foo._bar.A (with a space added between the ~ and the class name). I confirmed it does this with other themes too. I've looked around, but haven't found where or how this might be happening. Do you have any insights from your deeper Sphinx experience?

image

jbms commented 2 years ago

The ~ part is a hack that autodoc/the Sphinx python domain supports: if autodoc_typehints_format is set to "short" (the default), the sphinx.util.typing.stringify function that autodoc uses to convert Python types to their string representation will prefix the type name with a ~. The Sphinx Python domain then parses the text signature into an AST, and then when converting the AST into docutils nodes, treats the ~ in arguments and return types specially.

The difficulty here is that default values don't get treated the same as type annotations, because from the AST alone there is no way of knowing whether they are types or not. Instead the default value in the AST just gets "unparsed" back into its normal string representation, and then (provided by this theme specially) it gets syntax highlighted.

One solution might be:

Modify stringify_signature to use some special syntax to indicate a type. For example, something like SPHINX_IMMATERIAL_DEFAULT_VALUE_IS_TYPE[~foo._bar.A]. It needs to be valid Python syntax so that it can round trip through the AST. Then modify e.g. apidoc/python/style_default_values_as_code to detect this pattern and handle it appropriately.

Unfortunately since this solution relies on a special encoding of the default value, it doesn't handle the case where the signature is only available in text form, such as with pybind11 overloaded functions. For that case, we would either need to make pybind11 itself use the special encoding, or have a list of known types to match the default value against.

2bndy5 commented 1 year ago

In the past, I've been able to reference classes in type annotations by specifying the fully qualified name of the class (including module/pkg structure). Its been a long while since I tried using the ~ to abridge the type annotation for a class, but I seem to remember having to forego the ~. As an example, the CirPy_nRF24 docs use digitalio.DigitalInOut as a type annotation for a couple parameters in the constructor. And I didn't have to play around with python_type_aliases to get the digitalio.DigitaInOut to display as DigitaInOut in the parameters' field.

After re-reading this thread, I'm thinking that this isn't easily resolved with a robust solution, so I'm not sure how to move this forward.