-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Use SVG inheritance diagrams now that linking has been fixed #26567
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
Conversation
Great to hear! From currcent CI (e.g. https://output.circle-artifacts.com/output/job/49f53ea8-b87c-4ac6-9d04-9d6431a5a069/artifacts/0/doc/build/html/api/patches_api.html)
We definitively want to enforce sphinx >=7.2 with this, which means we should wait just a bit (7.2 is only 5 days old). |
I don't think it's a CI issue; the SVG are placed in |
Hmm, these both appear to be bugs, so I'll drop this PR to draft for now.
The generated SVG files have incorrect scale factors, which appears to be a bug in I'll point out that you are currently installing
It looks like there is something particular to matplotlib's configuration that is tripping up generating the correct links, so |
Lest I come across as a crazy person, here's an example from the |
Correct, something somewhere is getting confused about paths. I'll note that when the links are fixed, it's possible that CircleCI will prevent being able to follow the links. The tokening for accessing artifacts seems rather aggressive, so may not like links from embedded SVG files. However, this is not something to worry about until the links are actually fixed. |
The |
I have posted a PR to sphinx (sphinx-doc/sphinx#11634) that fixes the bugs with the links. (I have built the docs locally to confirm.) |
Thanks for pushing the issues upstream to Sphinx! Let’s wait with this PR until Sphinx has released a fix. |
I added a (temporary) commit to install sphinx from my fork to verify that the bugfixes work, which would be reverted once sphinx-doc/sphinx#11634 is merged and released. For example, https://output.circle-artifacts.com/output/job/7ec86fa7-c90e-4edf-8ec4-7571950c07a4/artifacts/0/doc/build/html/api/patches_api.html. Unfortunately, as I feared, it looks like CircleCI doesn't let you access linked pages from the SVG even when they are correct, so you have to manually inspect the links to see that they will work. |
sphinx-doc/sphinx#11634 is included in Sphinx 7.2.5, which was just released today. I've verified through building the docs locally that the inheritance-diagram links are correct, which can also be verified by inspecting the links in the CircleCI build (despite the fact that clicking on the links does not work). |
I've squashed and rebased on main. Let's see how this renders with current Sphinx. |
Looks like you didn't rebase against a current |
This reverts commit def9186.
Looks like this still works as intended to have SVG inheritance diagrams, e.g.: As before, clicking links in the CircleCI build doesn't work, so links have to be manually inspected. For example, the and the important part is that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Let's take this to our devdocs. We can always revert if we encounter issues.
PR summary
With sphinx-doc/sphinx#10614 and the to-be-merged sphinx-doc/sphinx#11634, links now work in SVG inheritance diagrams, including intersphinx links to external packages. This PR reverts PR #17913, which had switched the documentation to PNG inheritance diagrams.
PR checklist