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 contents entries for domain objects #10807

Merged
merged 31 commits into from Sep 13, 2022
Merged

Conversation

AA-Turner
Copy link
Member

@AA-Turner AA-Turner commented Sep 8, 2022

Closes #6316, closes #10804

Per a recent request on the CPython discord, this adds entries in the table of contents for domain objects (e.g. py:function, js:module, etc).

It does not support objects from the C, C++, and RST domains, as well as the option, cmdoption, and envvar objects.

A draft implementation by @agoose77 and @asmeurer was useful background, thank you both for this.

Feature or Bugfix

  • Feature

Relates

Ping for feedback and review, from people who have commented on the linked issues/CPython Discord thread: @Phillip-M-Feldman @slateny @pradyunsg @CAM-Gerlach @tony @agoose77 @asmeurer

A

@asmeurer
Copy link
Contributor

asmeurer commented Sep 8, 2022

Great, this saves me from having to turn that gist into a real Sphinx extension. Does this also include support for glossary entries?

I will test this out tomorrow.

@jakobandersen
Copy link
Contributor

Very nice. Can you outline the requirements for a domain to hook into / support this?

@n-peugnet
Copy link
Contributor

Could this change also allow the standard domain to provide objects, like the genindex document, the same way #10673 is trying to do it ?

@agoose77
Copy link

agoose77 commented Sep 8, 2022

I've not tried this out yet, but thanks for tackling it. I've just had no time of late, so I really appreciate someone pushing it over the line!

@AA-Turner
Copy link
Member Author

Does this also include support for glossary entries?

No, should it? Such a thing could be a follow-up PR.

A

@AA-Turner
Copy link
Member Author

Very nice. Can you outline the requirements for a domain to hook into / support this?

Define the fullname attribute on the first child of an addnodes.desc object, such that:

(isinstance(node, addnodes.desc) and bool(node[0]['fullname'])) is True

The python domain handles the definition in PyObject.handle_signature(), line 510.

A

@asmeurer
Copy link
Contributor

asmeurer commented Sep 8, 2022

I included glossary entries in the gist. It would be nice if they were also added to the toc, although whether it's here or in a separate PR doesn't really matter to me.

@asmeurer
Copy link
Contributor

asmeurer commented Sep 8, 2022

As I noted on the issue, I think we should keep the literal styling. This makes it easier to distinguish between functions and normal headers. Also, Sphinx always renders links to functions using literal styling, so this would be consistent with that.

@asmeurer
Copy link
Contributor

asmeurer commented Sep 8, 2022

As for showing the class name, I'm ambivalent. I can understand if it seems like too much, but it also makes it a little easier to see what's going on for classes with a lot of methods. Here's an example of part of the sidebar in the SymPy docs (built with my gist which includes the class name)
Screen Shot 2022-09-08 at 12 53 51 PM
Of course, if we can't decide on it, we can make it customizable.

I agree we shouldn't include the fully qualified name (although in general I wish Sphinx had better ways of letting you omit the fully qualified part of a function name in the docs).

ret: List[Node] = []

content_node: Element = nodes.section()
with switch_source_input(self.state, self.content):
Copy link
Contributor

Choose a reason for hiding this comment

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

Glad to see this fixed here too. I didn't look into it too deeply, but should PyModule just use the same mechanisms as the other Py* classes (subclassing from PyObject)?

Also I suppose the docs should be updated for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Was this addressed (documenting the change to py:module)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes:

image

A

@asmeurer
Copy link
Contributor

asmeurer commented Sep 8, 2022

I wonder if people would want this whole thing to be disableable with a configuration option. I guess if no one asks for it it's probably better to not add it, but it does seem like it could be something someone might not want.

@AA-Turner
Copy link
Member Author

Thanks all for reviews.

A

@mgeier
Copy link
Contributor

mgeier commented Sep 13, 2022

Thanks for implementing this, this is a great feature!

The structure of the domain object sections (using <dl>) is a bit different to "normal" sections (using <section>), but with a little tweak (mgeier/insipid-sphinx-theme#121), this works great with my theme (named insipid), including emphasizing the currently visible sections in the sidebar TOC.

For a live example, see https://insipid-sphinx-theme.readthedocs.io/en/sphinx5.x/showcase/api-doc.html

@mgeier
Copy link
Contributor

mgeier commented Sep 13, 2022

Interestingly, the domain object sections get a section number in the TOC (when using the :numbered: flag), while the sections in the main text don't get one.

Since the sections in the main text didn't have numbers in the first place, I guess I would expect the TOC entries to have no numbers either?

And the nesting in the reST section isn't correct, but the PR description already mentions that the reST domain is not supported.

@asmeurer
Copy link
Contributor

@AA-Turner thank you for doing the work to get this implemented in Sphinx proper. This is going to be a huge benefit to a lot of projects.

@AA-Turner
Copy link
Member Author

Thanks for testing this Matthias.

Interestingly, the domain object sections get a section number in the TOC (when using the :numbered: flag), while the sections in the main text don't get one.

Since the sections in the main text didn't have numbers in the first place, I guess I would expect the TOC entries to have no numbers either?

Please may you open a new issue for this, I'm not sure I fully understand? I'd like to get this fixed before 5.2 release

And the nesting in the reST section isn't correct, but the PR description already mentions that the reST domain is not supported.

I implemented support for the reST domain so nesting should work -- again please may you open a new issue with a mininal reproducer?

Thanks,
Adam

@mgeier
Copy link
Contributor

mgeier commented Sep 13, 2022

Please may you open a new issue for this, I'm not sure I fully understand?

#10826

@mgeier
Copy link
Contributor

mgeier commented Sep 13, 2022

I implemented support for the reST domain so nesting should work -- again please may you open a new issue with a mininal reproducer?

#10827

@domdfcoding
Copy link
Contributor

It is deliberate that the target node and HTML anchor are now after the docstring for modules? In the past the target was before the docstring, which (for docutils reasons I don't fully understand) made the anchor part of the section heading.

If I apply this patch, the target node and the anchor are in the positions they were before:

diff --git a/sphinx/domains/python.py b/sphinx/domains/python.py
index 6076eb7fb..bd507a21c 100644
--- a/sphinx/domains/python.py
+++ b/sphinx/domains/python.py
@@ -1024,7 +1024,7 @@ class PyModule(SphinxDirective):
             content_node.document = self.state.document
             nested_parse_with_titles(self.state, self.content, content_node)
 
-        ret: List[Node] = [*content_node.children]
+        ret: List[Node] = []
         if not noindex:
             # note module to the domain
             node_id = make_id(self.env, self.state.document, 'module', modname)
@@ -1045,6 +1045,7 @@ class PyModule(SphinxDirective):
             indextext = '%s; %s' % (pairindextypes['module'], modname)
             inode = addnodes.index(entries=[('pair', indextext, node_id, '', None)])
             ret.append(inode)
+        ret.extend(content_node.children)
         return ret
 
     def make_old_id(self, name: str) -> str:

@AA-Turner
Copy link
Member Author

@domdfcoding please may you open a new issue and tag me? This likely isn't intentional.

A

@jakobandersen
Copy link
Contributor

Why was the :noindex: option added back to the C and C++ domains? They don't support that feature (see #7906 and the originally raised issue).

@AA-Turner
Copy link
Member Author

Thanks Jakob--at one point in the PR I was using noindex and needed every domain to support it, I later switched to checking for noindexentry but clearly forgot to clean this up. The revert for this should go in to 5.2.3, if you don't get to it first I will make a PR over the weekend.

A

@jakobandersen
Copy link
Contributor

I don't think noindexentry is appropriate either, as it's already defined to control whether or not the object goes into the index page, which I believe is orthogonal to this feature. Rather make a new notocentry option for this control (and a global option for no toc entry on everything, for backwards compatibility).

@asmeurer
Copy link
Contributor

IIRC the implementation required handling noindex specially, but maybe that's a detail that can be worked around.

asmeurer added a commit to sympy/sympy that referenced this pull request Sep 30, 2022
ooliver1 added a commit to nextcord/nextcord that referenced this pull request Oct 1, 2022
sphinx-doc/sphinx#10807 decided to add a feature which was enabled by default.
This feature however increased the toctree's height and width by a huge amount - a breaking change.
This locks sphinx to an exact version to reduce issues like this again.
ooliver1 added a commit to nextcord/nextcord that referenced this pull request Oct 2, 2022
sphinx-doc/sphinx#10807 decided to add a feature which was enabled by default.
This feature however increased the toctree's height and width by a huge amount - a breaking change.
This locks sphinx to an exact version to reduce issues like this again.
DenverCoder1 pushed a commit to DenverCoderOne/nextcord that referenced this pull request Oct 23, 2022
sphinx-doc/sphinx#10807 decided to add a feature which was enabled by default.
This feature however increased the toctree's height and width by a huge amount - a breaking change.
This locks sphinx to an exact version to reduce issues like this again.
Bibo-Joshi added a commit to python-telegram-bot/python-telegram-bot that referenced this pull request Oct 25, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

automodule places members under docstring headers Create a ToC entry for every function, method, class, etc
9 participants