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

Add auto-generated API documention #810

Merged
merged 6 commits into from Sep 20, 2021
Merged

Conversation

bhrutledge
Copy link
Contributor

Related to #635.

While reviewing #799, I was curious to see how the Sphinx docstring directives would actually render, and how it treats typehints. This PR wires all of that up via the sphinx-apidoc command. I then merged in the new/updated docstrings as an example.

I'm not expecting this to be merged as-is. It feels a bit exotic, and since the audience is primarily maintainers and contributors, I expect that they will generally opt for reading the code rather than the docs. However, I did find it to be a useful preview of the docstrings, and it helped me think about how to write/organize them.

Maybe this could live in a separate branch, rendered as a distinct version on ReadTheDocs (along with latest and stable)?


# TODO: Try to add these to intersphinx_mapping
nitpick_ignore_regex = [
(r"py:.*", r"(pkginfo|requests|IO).*"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't we add the intersphinx mapping for requests? It should be as simple as updating 280 to be

{
  "python": ("https://docs.python.org/", None),
  "requests": ("https://requests.readthedocs.io/", None),
}

See also: https://www.sphinx-doc.org/en/master/usage/extensions/intersphinx.html

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried that, via:

intersphinx_mapping = {
    "python": ("https://docs.python.org/", None),
    "requests": ("https://docs.python-requests.org/en/latest/", None),
}

But it's not entirely working:

WARNING: py:class reference target not found: requests.models.Response

Via intersphinx-untangled, it seems that the documented reference is the public requests.Response, and even though that's the type hint, it's being resolved to the internal class definition.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't we just use requests.models.Response? It's documented as that in the docs. Most people use requests.Response because the author wanted to re-export the world which was a bad idea anyway

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't we just use requests.models.Response? It's documented as that in the docs.

Where do you see it documented that way? I only see https://docs.python-requests.org/en/master/api/#requests.Response.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went down the rabbit hole on this, with the end result being sphinx-doc/sphinx#9653.

twine/utils.py Outdated Show resolved Hide resolved
@bhrutledge bhrutledge force-pushed the 635-apidoc branch 2 times, most recently from ecd3bd7 to 04f19f6 Compare September 15, 2021 10:50
@bhrutledge bhrutledge marked this pull request as ready for review September 20, 2021 01:33
@bhrutledge bhrutledge merged commit 28d8571 into pypa:main Sep 20, 2021
@bhrutledge bhrutledge deleted the 635-apidoc branch September 20, 2021 01:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants