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

[docs] Fix Sphinx issue with linkless summaries #3284

Merged
merged 4 commits into from
Nov 5, 2024

Conversation

johnkerl
Copy link
Member

@johnkerl johnkerl commented Nov 2, 2024

Issue and/or context: Ongoing work as part of #3282. Also fixes #3227.

Problem

What's right

Screenshot 2024-11-02 at 1 21 16 PM

Screenshot 2024-11-02 at 1 21 32 PM

What's wrong

  • Simply go one level deeper: scroll down to "Methods"
  • Hover over a method, e.g. add_new_collection
  • This is not a hyperlink
  • What you see is the first sentence of its docstring
  • Nothing you can click on will show you the rest of the docstring

Screenshot 2024-11-02 at 1 24 04 PM

Analysis

At the first level we have hand-written .rst like this:

We keystroke out

.. autosummary::
    :toctree: _autosummary/

At the next level, we're letting autogen recurse for us and create .rst files. The .rst files that are auto-generated are missing this line:

    :toctree: _autosummary/

I spent the morning debugging, experimenting with sphinx-autogen and sphinx-apidoc, copy/pasting examples out of the Sphinx docs, playing with the :recursive: tag, doing pip install -U of every Sphinx package on my system, etc. -- and I came to the following conclusions:

  • I could not find any syntax that would make the autogenerator emit the :toctree: dirnamegoeshere line on the .rst files it generates.
  • Therefore I autogenerated the "next level in", manually edited those to include the :toctree: dirnamegoeshere lines, and am adding them to source control.

Changes:

As above.

Notes for Reviewer:

This means more manual editng when we create new classes/methods. However, there is already a non-zero amount of manual edits necessary: #3283 is a case in point. I would love to have fully automated/hands-off code -> doc generation but we simply do not have that at present. I will take a method with one more manual step, that produces correct and useful documentation, over a method one with one fewer manual step that produces incomplete and misleading documentation.

To view the docs: please compare

Note this second link is on what readthedocs calls a "hidden branch" which I created as a manual one-off for this PR review.

PR stacking as of today:

Copy link

codecov bot commented Nov 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.40%. Comparing base (762abda) to head (00839da).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3284      +/-   ##
==========================================
+ Coverage   85.29%   85.40%   +0.10%     
==========================================
  Files          52       52              
  Lines        5517     5517              
==========================================
+ Hits         4706     4712       +6     
+ Misses        811      805       -6     
Flag Coverage Δ
python 85.40% <ø> (+0.10%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
python_api 85.40% <ø> (+0.10%) ⬆️
libtiledbsoma ∅ <ø> (∅)

Base automatically changed from kerl/readthedocs-pre-1.15 to main November 4, 2024 14:25
@ivirshup
Copy link
Collaborator

ivirshup commented Nov 4, 2024

I will try to take a closer look later in the week and see if I can remember how to get autogenerated class docs working. But in the meantime, I noticed that the CSS for the class listing seems to be off in the demo build: https://tiledbsoma.readthedocs.io/en/kerl-fix-sphinx-linkless-summaries/python-tiledbsoma.html#classes

image

@johnkerl
Copy link
Member Author

johnkerl commented Nov 4, 2024

Thanks @ivirshup !

Can you please be more specific about

CSS for the class listing seems to be off in the demo build:

? I don't know what to try to fix ...

@johnkerl johnkerl force-pushed the kerl/fix-sphinx-linkless-summaries branch from 8a99f68 to 00839da Compare November 5, 2024 16:19
Copy link
Member

@ryan-williams ryan-williams left a comment

Choose a reason for hiding this comment

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

Approving to unblock, will try to figure out how to get Sphinx to auto-generate the things we want it to, async…

@johnkerl johnkerl merged commit 1b8cd98 into main Nov 5, 2024
25 checks passed
@johnkerl johnkerl deleted the kerl/fix-sphinx-linkless-summaries branch November 5, 2024 19:32
@johnkerl
Copy link
Member Author

johnkerl commented Nov 5, 2024

Approving to unblock, will try to figure out how to get Sphinx to auto-generate the things we want it to, async…

This is great, thanks @ryan-williams !!

Also recall as I noted above, I don't entirely dislike having to spell more things out -- it lets us put methods in more intuitive order.

And honestly, the more I think about it, maybe I don't even really want fully autogenerated docs as much I had (very recently!) thought I did ... having an environment where manual curation is allowed and encouraged, I feel like this will improve the quality of the docs ......

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.

[Bug] Class methods do not have all desired material in rendered doc pages
3 participants