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

feature: link to re-export instead of inserting full documentation #167

Open
JP-Ellis opened this issue Mar 28, 2024 · 5 comments
Open

feature: link to re-export instead of inserting full documentation #167

JP-Ellis opened this issue Mar 28, 2024 · 5 comments
Assignees
Labels
feature New feature or request fund Issue priority can be boosted insiders Candidate for Insiders

Comments

@JP-Ellis
Copy link

JP-Ellis commented Mar 28, 2024

Is your feature request related to a problem? Please describe.

Let's suppose we have a few related classes which are written in separate files, but for convenience are exposed higher up:

dog/
- __init__.py
- chihuahua.py
- pug.py
- labrador.py
- golden_retriever.py

with the main __init__.py file looking something like:

from .chihuahua import ChihuahuaDog
from .pug import PugDog
from .labrador import LabradorDog
from .golden_retriever import GoldenRetrieverDog

__all__ = [
  "ChihuahuaDog",
  "PugDog",
  "LabradorDog",
  "GoldenRetrieverDog"
]

It would appear that Griffe documents each of these classes twice: once where they are defined, and second time for their re-export through dog/__init__.py.

The duplicated docs are unnecessary and can be a little confusing at first.

Describe the solution you'd like

A way to avoid the duplication and have each class be documented only once.

A nice solution I have seen would be to have a list of re-exports in the same way that the Rust docs do it. See here and here for some examples. Note that the latter example is a common practice in Rust to have a module called prelude whose sole purpose is to re-export commonly used types.

If it isn't possible for Griffe to detect re-exports, an alternative might be to have some sort of in-file documentation, or perhaps a separate configuration.

Describe alternatives you've considered

The only alternatives I can image are either to:

  • Live with the duplication, or
  • Don't put re-exports in __all__ in which case the re-export is entirely hidden.

Boost priority

  • Boost priority in our backlog through Polar.sh. Higher pledge, higher priority.
  • Minimum pledge by user/organization is $5, minimum amount for boost is $30.
  • View all issues with pledges.
  • We receive the funds once the issue is completed and confirmed by you.
  • Features with the insiders label are released to sponsors first, and tied to a funding goal.
Fund with Polar
@JP-Ellis JP-Ellis added the feature New feature or request label Mar 28, 2024
@JP-Ellis JP-Ellis changed the title feature: link to re-export instead of inserting full docuimentation feature: link to re-export instead of inserting full documentation Mar 28, 2024
@pawamoy
Copy link
Member

pawamoy commented Mar 28, 2024

Hey, thanks for the feature request!

So IMO this has more to do with mkdocstrings-python than Griffe. IMO again, Griffe does the correct thing here: it loads everything it can from your package. And it actually knows that PugDog and siblings are re-exported from the top-level module. It therefore knows that PugDog and siblings actually have two public locations. One location in the top-level module, and another location in each submodule.

Now, mkdocstrings-python could try to be a bit more clever than it is right now, and guess the intent of the user, which is to render PugDog and siblings only once, probably from the highest public location.

But, what should it do with the other locations? Hide them? Render links?
Also, why should it even try to be clever, when Python devs have ways of disambiguating their public APIs, for example by marking their submodules as private (_pug.py), especially if users are not meant to import from submodules, but rather from the top-level module? Why should it when docs writer have ways to select which objects and their members to render on which page?

What I mean is that, currently, I believe you are opting in the "render everything recursively" behavior. If you only mean to render docs for objects exposed in the top-level module, you should then inject docs only for the top-module, and not for any submodules. Or if you expose things in submodules, then you should be able to manually exclude (or even automatically exclude with some scripting, as Griffe provides the necessary API) some objects that were already rendered from the top-level module. mkdocstrings-python provides all the necessary options for that too.

I'm not saying that what you want is wrong: I actually agree with you and have the same use-case (docs for Griffe render some class twice, in the top-level module page and in submodule pages)! What I'm saying is that it is hard to accomplish what you want in a way that will please everyone, because there is no standard for that. We only have weak conventions (like prefixing names with _ and hijacking __all__ to mark objects as public).

So, with all this in mind, I think the only solution we can implement is an option in mkdocstrings-python that says "render links to higher locations if they exist". I don't think it will cater to everyone needs, but it's a start.

@JP-Ellis
Copy link
Author

Thank for the response. You raise some good points, and let me respond to them :)

So IMO this has more to do with mkdocstrings-python than Griffe.

That's good to know, and I wasn't aware! I might make a comment there instead.

But, what should it do with the other locations? Hide them? Render links?

I think the best option is to render a link. Having used Rust a fair bit, I especially like the idea of a section of re-exports (see links to examples in my original post):

image

Hiding them could be an option, but I don't like it as a default option, especially when the ability to list re-exports is simple, elegant and unobstrusive.

Also, why should it even try to be clever, when Python devs have ways of disambiguating their public APIs, for example by marking their submodules as private (_pug.py), especially if users are not meant to import from submodules, but rather from the top-level module? Why should it when docs writer have ways to select which objects and their members to render on which page?

These are good points, and I agree there is no "one size fits all" solution here. Let me do a bit of a brain dump and consider a few scenarios. I'm using the example of a public class, but the same would apply to other documented objects.

  1. Public class declared in public module

    This is the most straightforward, and what I would simply do is document the class in the public module only. All other re-exports in other modules are listed as re-exports and link to the class definition.

  2. Public class declared in private module and re-exported elsewhere

    This is where things quickly become murky. From my perspective, the priority is to have a single definition that acts as if it were the 'true' declaration and all other re-exports link to it. For example, consider a scenario with the following declarations:

    my_pkg.foo._foobar.FooBar  # actual definition
    my_pkg.foo.FooBar          # Re-export
    my_pkg.bar.FooBar          # Re-export
    my_pkg.FooBar              # Re-export

    I can imagine two algorithms to determine which re-export to act as if it were the 'true' declaration:

    • Bottom up: Starting from the my_pkg.foo._foobar.FooBar, travel up the tree and stop at the first re-export of FooBar and use that. So in the above example, it would be my_pkg.foo.FooBar and the other two re-exports would link back to it. If the class is never re-exported in any of the module's parent, then the class goes undocumented.

    • Top down: Starting from my_pkg, travel down the tree towards my_pkg.foo._oobar.FooBar and stop at the first re-export of FooBar. So in the above example, it would be my_pkg.FooBar, and the other two re-exports would like back to it. If the class is never re-exported in any of the source module's parent, then the class goes undocumented.

    My strong preference between these two is the bottom up approach, as it reflects most closely how the source code is actually organised. Furthermore, the top down approach has the risk of creating a very polluted top-level module for packages which re-export a lot of the functionality at the top level.

    I also acknowledge that neither algorithm is guaranteed to produce a result. In particular, both algorithms would fail under this scenario:

    my_pkg.foo._foobar.FooBar  # actual definition
    my_pkg.bar.FooBar          # Re-export
    my_pkg.baz.FooBar          # Re-export

    If the algorithm fails, I think it would be fine to raise a warning or an exception. I also don't think there's an elegant alternative to decide between bar.FooBar and baz.FooBar (therefore justifying the warning/exception).

Also to address your point that developers have the option to mark their submodules as private, that is perfectly valid. In fact, it may even be beneficial to raise a warning if a class is being repeatedly documented?

So, with all this in mind, I think the only solution we can implement is an option in mkdocstrings-python that says "render links to higher locations if they exist". I don't think it will cater to everyone needs, but it's a start.

I agree, and starting with something is better than nothing. If people can give feedback, then it can also help determine how to take it forward.

I would just reverse the option to "render links to lower locations if they exist" (as mentioned above, to both avoid polluting the top modules, and to reflect more closely how the underlying code is structured).

@pawamoy
Copy link
Member

pawamoy commented Mar 30, 2024

Awesome feedback, thanks a lot! I agree with the bottom-up approach 🙂 Lets move forward with it (it'll still be optional). Also the idea to warn about objects having multiple public locations is great! I'll implement that too, in griffe check 🙂

@JP-Ellis
Copy link
Author

Let me know if there's anything I can do, whether it be in code contributions, PR reviews, or testing 👍

@pawamoy
Copy link
Member

pawamoy commented Jun 8, 2024

Moving this to mkdocstrings-python 🙂

@pawamoy pawamoy transferred this issue from mkdocstrings/griffe Jun 8, 2024
@pawamoy pawamoy added the insiders Candidate for Insiders label Aug 20, 2024
@pawamoy pawamoy added the fund Issue priority can be boosted label Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request fund Issue priority can be boosted insiders Candidate for Insiders
Projects
None yet
Development

No branches or pull requests

2 participants