sphinx-doc / sphinx

The Sphinx documentation generator
https://www.sphinx-doc.org/
Other
6.55k stars 2.12k forks source link

Consider class aliases when generating `:py:meth:` or `:py:attr:` links. #12863

Open defnull opened 1 month ago

defnull commented 1 month ago

Autodoc correctly detects class aliases (two names referencing the same class) and generates a short alias of OtherClass hint instead of redundant API documentation. References to members of that alias class however are not forwarded to the original class as one would expect.

Example:


class LongClassName:
    def foo(self): pass

ShortName = LongClassName

This correctly emits a short "alias of LongClassName" summary instead of redundant API documentation:

.. autoclass:: ShortName
   :members:

This correctly generates API documentation for `LongClassName`:

.. autoclass:: LongClassName
   :members:

I can now reference :meth:`LongClassName.foo` but not :meth:`ShortName.foo`.

Describe the solution you'd like

Referencing a members of an alias class should link to the corresponding member of the original class as a fallback. Given the example above, :meth:`ShortName.foo` should automatically link to LongClassName.foo and not generate a broken link, as it currently does.

electric-coder commented 1 month ago

Given the example above, :meth:`ShortName.foo` should automatically link to LongClassName.foo and not generate a broken link.

It shouldn't generate a broken link, but it should link to the declaration of ShortName.foo. When you declare something in rst it's an explicit domain declaration and those should exist and be possible to cross-reference regardless if they're aliases in Python. Otherwise users wanting the opposite behavior wouldn't be able to implement it; in your case writing :meth:`ShortName.foo <LongClassName.foo>` would be the correct solution.

defnull commented 1 month ago

It currently does generate a broken link because there is no declaration for ShortName.foo. Autodoc generates member declarations only for the original class, not if it's an alias. In the example above, .. autoclass:: ShortName generates a short "alias of LongClassName" description and that's it. The :members: option is ignored on alias classes, which is desired because otherwise all documentation would be generated twice (with different class names).

Sure, using the long :meth:`ShortName.foo <LongClassName.foo>` syntax would work, but that's easy to forget and quite tedious. If I reference a member of an alias class, I usually want to link to something meaningful, and that would be the declaration of the aliased class in almost all cases. So, linking to the original if there is no explicit declaration for an aliased class member would be a sensible default, no?

electric-coder commented 1 month ago

but that's easy to forget and quite tedious

It's generally expected of devs they know how to write their code, it's a common misconception they needn't know how to write their docs. As for the shortened cross-reference syntax that's a common FR (I've lost count of how many posts I've seen about the issue) but it comes with a number of hidden implicit vs explicit drawbacks if used.

.. autoclass:: ShortName generates a short "alias of LongClassName" description and that's it.

That's necessary because you could be writing something that recursively has to extract members without additional declarations thus adding the alias automatically is desirable, like:

.. automodule:: the_module
   :members:

But you should be able to write an explicit declaration if you really need to escape some of the automatic behavior (I haven't tested it but this should omit the automatic alias reference) like:

.. autoclass:: LongClassName
   :members:

.. py:class:: ShortName

   Write anything you want here, this declarartion shouldn't have the default alias text.
defnull commented 1 month ago

Then we disagree, which is fine. I think that, when referencing a member of an alias- (or sub-) class, an automatic fallback to the known declaration of the original (or parent) class member is a sensible default if there is no explicit declaration for the exact member I am referencing and it would otherwise generate a broken reference. This is what I (as a developer/author) would expect, because I know that sphinx knows that the member I am referencing is an alias (or inherited member) for something that has a declaration and can be linked to.

defnull commented 1 month ago

Maybe this is just what I am used to, which is why I expect the same behavior from sphinx. In Javadoc for example, when referencing a method of a sub-class that does not actually declare that method, the link naturally points to the closest parent class that does. The syntax {@link Child#method()} links to Parent#method() unless method is also declared on Child. In Sphinx, :meth:`Child.method` would generates a broken reference instead.

electric-coder commented 1 month ago

Then we disagree, which is fine.

It's not a matter of disagreeing but of objective analyses. As I've said: I've seen this FR a hundred times for every use case we care to image.

The syntax {@link Child#method()}

There isn't any method on the child, the method is defined on the parent. So Sphinx takes the exact approach which is optimal.

In Javadoc for example (...) the link naturally points to the closest parent class that does.

That's a thinking pattern coming from a background in compiled languages. Python has multiple and dynamic inheritances, both mechanisms that Java doesn't have. So wanting to write a cross-reference to something that isn't defined and expecting an automatic resolution sounds simple but: it would inevitably result in poorly written rst. So the motivation is, after careful analyses, needless.

To sum it up: Why would Sphinx fail with a cross-reference to something that isn't declared? Because it should.

This is what I (as a developer/author) would expect

And that's the catch, Python does dynamic resolution but reStructuredText isn't a language that's supposed to do any resolution beyond the scope of it's own declarations.

defnull commented 1 month ago

It's not a matter of disagreeing but of objective analyses. As I've said: I've seen this FR a hundred times for every use case we care to image.

Which sounds like a pretty strong indication that sphinx does not to what users expect.

Anyway, your opinion seems to be final. I'd like to keep this open and hear some more opinions, but you can also move forward and close this issue as wont-fix if you have the authority to make such decisions.

electric-coder commented 1 month ago

I'd like to keep this open and hear some more opinion

There are about 100 FRs in this repository asking for the same thing. Most are posted by new users with little experience in Sphinx expecting magic from the cross-reference syntax bending reST to the Python logic (and here's another catch: cross-references aren't just for documenting Python, and other languages don't have Python's MRO). You can go ahead, do some research, and write a comprehensive overview thread (there have to be some 200 other threads on Stack Overflow asking the same thing, and I've also read them all).

electric-coder commented 1 month ago

Sphinx does not to what users expect.

The cross-reference syntax has 4 parts and users have expected/requested each part to be different, namely:

:domain:role:`title <target>`

FRs come in 4 kinds:

  1. The :domain: part should be different: (I've omitted it and now I have bugs - magic).
  2. The :role: part should be different; (I'd like to write :any: or just nothing and it should always work - magic).
  3. The title part should be different; (Just write anything and let Python resolve it - magic).
  4. The <target> part should be different; (Omit it; make it implicit; write to any depth; let Python resolve it - magic.)

Of the above I'd only support the "write qualified name to any depth".

Your FR is a 3. and 4. You don't want to write a title - that's 3. And because of 3 (here it becomes an XY problem): you want to cross-reference a non-existing target, that isn't declared in the index, and that it resolve - that's 4.

What these FRs never acknowledge is that their end is a poor software engineering practice.