jbms / sphinx-immaterial

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

Strange overload signature with v0.12.1 #374

Open mhostetter opened 1 month ago

mhostetter commented 1 month ago

Hey guys! I saw there was a flurry of activity to get to v0.12. Thanks for that! I just upgraded to it, but noticed a strange artifact in an overloaded signature in one of my projects.

I have this signature:

@overload
def time_domain(
    x: npt.NDArray,  # TODO: Change to npt.ArrayLike once Sphinx has better overload support
    *,
    sample_rate: float | None = None,
    centered: bool = False,
    offset: float = 0.0,
    ax: plt.Axes | None = None,
    diff: Literal["color", "line"] = "color",
    **kwargs,
): ...

@overload
def time_domain(
    t: npt.NDArray,  # TODO: Change to npt.ArrayLike once Sphinx has better overload support
    x: npt.NDArray,  # TODO: Change to npt.ArrayLike once Sphinx has better overload support
    *,
    sample_rate: float | None = None,
    centered: bool = False,
    offset: float = 0.0,
    ax: plt.Axes | None = None,
    diff: Literal["color", "line"] = "color",
    **kwargs,
): ...

def time_domain(  # noqa: D417
    *args,
    sample_rate: float | None = None,
    centered: bool = False,
    offset: float = 0.0,
    ax: plt.Axes | None = None,
    diff: Literal["color", "line"] = "color",
    **kwargs,
):

It was rendered like this in v0.11.14 with Sphinx v7.3.7, which I'm pretty happy with. https://mhostetter.github.io/sdr/v0.0.22/api/sdr.plot.time_domain/

image

But in v0.12.1 and Sphinx v7.3.7, it renders like this. https://mhostetter.github.io/sdr/v0.0.x/api/sdr.plot.time_domain/

image

Have any idea what might be causing that?

2bndy5 commented 1 month ago

Seems like our new support for type parameters (supported in Sphinx v7.3+) has taken an affect. It seems like npt.NDArray is a generic type with a type parameter set to _Scalar_type_co.

~I think Sphinx's new ability to show metadata on an Annotated type (introduced in Sphinx v7.4) is also taking affect here, but it doesn't look like ndarray inherits from the Annotated class. The C source for makes it difficult to determine this.~

jbms commented 1 month ago

I take it the problem is the poor line wrapping?

Our wrap_signatures_with_css option currently also wraps the type parameters if it determines that wrapping is needed. It could be updated but in general it seemed like a losing game to try to reimplement proper wrapping.

Instead I added the option to format using black. Unfortunately black also has an outstanding bug with type parameters but hopefully that will be fixed soon. https://github.com/psf/black/issues/4071

mhostetter commented 1 month ago

I take it the problem is the poor line wrapping?

My concern was why the [_ScalarType_co] was showing up at all. I didn't know about Python's new type parameters. But since my signature doesn't use type parameters, would it make sense to suppress whatever recognized that type parameter, such that it is no longer documented?

jbms commented 1 month ago

Currently type parameters are detected automatically even if you don't use the new python 3.12 syntax. We could add an option to disable that, I suppose. Is that preferable to fixing the formatting?

mhostetter commented 1 month ago

Should the type parameters appear at all in my case? There's no output to the function. And it's not parameterized by any types. It just takes a npt.NDArray as an input.

A global disable switch would prevent this signature from (in my opinion) erroneously showing the type parameter, but the downside is all intentional type parameters would also be disabled. I wonder if my example is a bug in how the type parameters are/should be displayed.

2bndy5 commented 1 month ago

I think that since the type parameter in this case starts with a _, it probably should be disregarded.

I think it's worth automatically hiding the type parameter for specialized types, when a generic type's parameter is assigned a known type. It might be harder to detect what a known type is though. Not all libraries are compatible with sphinx' introspection.

jbms commented 1 month ago

Should the type parameters appear at all in my case? There's no output to the function. And it's not parameterized by any types. It just takes a npt.NDArray as an input.

A global disable switch would prevent this signature from (in my opinion) erroneously showing the type parameter, but the downside is all intentional type parameters would also be disabled. I wonder if my example is a bug in how the type parameters are/should be displayed.

I see. It gets detected as having type parameters because the signature displays with type parameters even though you didn't specify any explicitly. That is presumably due to a limitation in generic type aliases prior to python 3.12. In fact I'm surprised it displays as NDArray at all rather than what it evaluates to.

I think in this case you could just specify the type as numpy.ndarray and that would fix the problem.

As far as making ArrayLike display better could try doing something like this:

https://github.com/google/neuroglancer/blob/3efc90465e702453916d2b03d472c16378848132/docs/conf.py#L202

2bndy5 commented 1 month ago

I'm surprised it displays as NDArray at all rather than what it evaluates to

According to numpy-typing docs, NDArray (a type alias) is being resolved (as defined numpy.ndarray[typing.Any, numpy.dtype[+ScalarType]]).

jbms commented 1 month ago

I'm surprised it displays as NDArray at all rather than what it evaluates to

According to numpy-typing docs, NDArray (a type alias) is being resolved (as defined numpy.ndarray[typing.Any, numpy.dtype[+ScalarType]]).

Oh yes, I see that it is. I misremembered the screenshot when I sent that message.

2bndy5 commented 1 month ago

You're rather unapologetic when it comes to monkey patching python's runtime. I've never seen it wielded so freely in any other python projects.

jbms commented 1 month ago

Monkey patching has a lot of downsides but sometimes it is the most practical option, especially when sphinx is involved.

For these numpy types I considered adding some logic in our type annotations transform code but just monkey patching numpy to ensure the aliases stayed unexpanded seemed much simpler. Since it only applies when building the documentation it doesn't even matter if it breaks things as long as those things aren't needed when building the documentation.