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.

Sign up for GitHub

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

Jump to bottom

Fix several issues with nav_adapt and add unit tests #139

Merged
merged 8 commits into from
Jul 28, 2022
Merged

Fix several issues with nav_adapt and add unit tests #139

merged 8 commits into from
Jul 28, 2022

Conversation

jbms
Copy link
Owner

@jbms jbms commented Jul 26, 2022

Fixes #137.
Fixes #138.

Also fixes support for only directives that affect sections.

@jbms jbms requested a review from 2bndy5 July 26, 2022 23:36
@2bndy5
Copy link
Collaborator

2bndy5 commented Jul 27, 2022

I can confirm this fixes #138 and allows .. only:: (tested w/ html as arg) applied to .. toctree::.

@mhostetter can you confirm #137 is satisfied? I tested it with our autodoc example, but not with the apigen example.

@jbms I'm noticing the first entry in the global toc is missing for sub root pages.
For example "General API customization" page isn't listed in the global toc
image
but I can get to it by clicking "Formatting signatures" and hitting the "Previous" button at the bottom. I first noticed this when the demo_api page wasn't listed in the examples pages.

@mhostetter
Copy link
Contributor

I'm noticing the first entry in the global toc is missing for sub root pages.

I noticed this too.

@mhostetter can you confirm #137 is satisfied? I tested it with our autodoc example, but not with the apigen example.

With include_rubrics_in_toc=True, I'm noticing that all the rubrics that were added to the TOC are now removed from both the left- and right-side TOCs (which is equivalent behavior to include_rubrics_in_toc=False).

Class example

image

Function example

image

@2bndy5
Copy link
Collaborator

2bndy5 commented Jul 27, 2022 •
edited
Loading

when I added

.. rubric:: A rubric

to the autodoc'd class' docstr, and set

object_description_options = [
    ("py:.*", dict(include_rubrics_in_toc=True)),
]

The rubric showed in the local toc:
image
I'm not sure if that's what was requested.

@mhostetter
Copy link
Contributor

I'm not sure if that's what was requested.

That is the desired behavior. Not sure why I'm seeing something different. Perhaps it's the difference between autodoc and python-apigen. I'll have to do more testing.

@jbms
Copy link
Owner Author

jbms commented Jul 27, 2022

Thanks for the testing. The issue with include_rubrics_in_toc was just that I had accidentally based my branch on an older commit before include_rubrics_in_toc was merged in. I have now rebased it.

Additionally, I have fixed the issue whereby the first child of a toctree with a :caption: was hidden.

Separately, I refactored the python domain fixes to split them into separate files, as was done previously for C++. As part of that refactor, I ensured that all monkey patching happens at module init time rather than during the setup function, to avoid issues if setup is called more than once (e.g. during unit tests where we build more than once per process).

@jbms
Copy link
Owner Author

jbms commented Jul 27, 2022

I can confirm this fixes #138 and allows .. only:: (tested w/ html as arg) applied to .. toctree::.

To be clear, while it will build with only applied to a toctree, due to the linked Sphinx bug the only directive has no effect on the global toc --- the toctree is always included in the global toc regardless of the condition.

@mhostetter
Copy link
Contributor

Thanks for the testing. The issue with include_rubrics_in_toc was just that I had accidentally based my branch on an older commit before include_rubrics_in_toc was merged in. I have now rebased it.

Thanks. It does appear that the rubrics are working as intended. See below that the "Examples" rubric is suppressed from the global TOC, which was the goal of #137. 👍

I did notice, though, that with python-apigen object pages now seem to have an extra heading. See below that the galois.BCH.generator_poly page with the yellow "P" marker appears as the first heading in the local TOC and generator_poly without the yellow "P" is added to the global TOC. The same is true for the extra heading in the galois.BCH page.

With `main`

image

With 488e35f

image

jbms added 7 commits July 27, 2022 16:47
@jbms
Refactor Python domain and monkey patching to prevent issues in unit …
80acb31 
…tests

This commit separates all of the Python domain fixes into separate
files, as was done for C++.

Additionally, this commit ensures that any monkey patching is done
during module initialization rather than during extension setup, to
avoid applying the same monkey patch more than once if there is more
than one build performed in the same Python process, as happens during
unit tests.
@jbms
Fix formatting of tests and include in CI mypy/black run
7bba915
@jbms
Fix regression in URLs in local toc for root document
6aaa8ed 
This fixes a regression introduced by
87d87f9.

Fixes #138.
@jbms
Don't add extra useless tooltip to document refs in TOC
9178868 
Previously, regular links to documents were unintentionally given a
tooltip of `<pagename> (document)`, which was not helpful.  This
happened because `doc` targets are listed as Sphinx "objects".
@jbms
Exclude local sections of parent pages from global toc
931df43 
Fixes #137.
@jbms
Support only directives that impact the TOC
0f12699 
This addresses one issue noted in #132 although due to Sphinx bug
sphinx-doc/sphinx#9819 it is still not
possible to use only directives to selectively include `genindex` and
`modindex` in the global TOC.
@jbms
Add unit tests for nav_adapt and Python domain fixes
f889aad
@jbms
Copy link
Owner Author

jbms commented Jul 27, 2022

I did notice, though, that with python-apigen object pages now seem to have an extra heading. See below that the galois.BCH.generator_poly page with the yellow "P" marker appears as the first heading in the local TOC and generator_poly without the yellow "P" is added to the global TOC. The same is true for the extra heading in the galois.BCH page.

This was due to a bug in my refactoring of the Python domain monkey patches, which broke nonodeid (used by Python apigen). That is now fixed and I added a unit test.

@mhostetter
Copy link
Contributor

This was due to a bug in my refactoring of the Python domain monkey patches, which broke nonodeid (used by Python apigen). That is now fixed and I added a unit test.

Awesome, thanks. I can confirm this works for me now. 👍

Separately, I refactored the python domain fixes to split them into separate files, as was done previously for C++.

I like the refactoring -- makes the code easier to follow.

2bndy5
2bndy5 approved these changes Jul 28, 2022
View reviewed changes
Copy link
Collaborator

@2bndy5 2bndy5 left a comment

Choose a reason for hiding this comment

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

I also appreciate the refactor to bite-size modules. It makes reviewing much more palatable.

pytest snapshots looks like a huge time saver!

sphinx_immaterial/apidoc/format_signatures.py Show resolved Hide resolved
sphinx_immaterial/apidoc/python/default.py Show resolved Hide resolved
@jbms jbms merged commit bf16975 into main Jul 28, 2022
@jbms jbms deleted the fix-138 branch July 28, 2022 00:51
@jbms
Copy link
Owner Author

jbms commented Jul 28, 2022

Thanks for the review!

@2bndy5
Copy link
Collaborator

2bndy5 commented Jul 28, 2022

Think I'll push v0.8.1

@jbms
Copy link
Owner Author

jbms commented Jul 28, 2022

Think I'll push v0.8.1

Sounds good

@jbms jbms mentioned this pull request Aug 17, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@2bndy5 2bndy5 2bndy5 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@jbms @2bndy5 @mhostetter