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

Fixes missing and broken links in inheritance diagrams #10614

Merged
merged 10 commits into from
Aug 8, 2023

Conversation

ayshih
Copy link
Contributor

@ayshih ayshih commented Jun 29, 2022

Subject: Fixes missing and broken links in inheritance diagrams

Feature or Bugfix

  • Bugfix

Purpose

There are multiple bugs that result in missing or broken links in inheritance diagrams. The missing links are due to bugs that break the correct association of external classes with URLs. The broken links are specific to SVG output where the relative paths are not correctly re-pathed. This PR has been rewritten given #11078.

Detail

Relates

@ayshih
Copy link
Contributor Author

ayshih commented Jun 29, 2022

This PR adds more fixes to my already open PR #10576. You can decide whether to review that one first or to simply review this one.

@ayshih
Copy link
Contributor Author

ayshih commented Sep 27, 2022

I've gone ahead and closed #10576 so that there's just a single PR to review (i.e., this one).

Can I please get some feedback on this PR? Thanks!

@AA-Turner
Copy link
Member

Can you change the base to 5.x?

A

@ayshih ayshih changed the base branch from 5.0.x to 5.x September 27, 2022 15:42
@ayshih
Copy link
Contributor Author

ayshih commented Sep 27, 2022

Okay, changed and rebased to 5.x

Copy link
Member

@AA-Turner AA-Turner left a comment

Choose a reason for hiding this comment

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

Please avoid pathlib for now, and add tests for the changed behaviour. You also need to add an entry to CHANGES.

A

sphinx/ext/inheritance_diagram.py Outdated Show resolved Hide resolved
sphinx/ext/inheritance_diagram.py Outdated Show resolved Hide resolved
sphinx/ext/inheritance_diagram.py Outdated Show resolved Hide resolved
@ayshih ayshih force-pushed the fix_inheritance_diagrams branch 3 times, most recently from 062ca0b to 207cdec Compare October 1, 2022 03:08
@ayshih
Copy link
Contributor Author

ayshih commented Oct 1, 2022

Apologies for the noise about the fuzzy matching! When crafting all of the tests, I discovered that the issue with fuzzy matching is actually an interaction with a third-party extension (sphinx-automodapi). I've accordingly ripped out that part of the PR, and the remaining fixes are all for bugs in first-party Sphinx.

@ayshih
Copy link
Contributor Author

ayshih commented Oct 1, 2022

Tests all green, PR ready for re-review, thanks!

@ayshih
Copy link
Contributor Author

ayshih commented Nov 2, 2022

@AA-Turner: I see that you changed the base branch to master, so I've rebased this PR and moved the CHANGES entry to be under 6.0.0 (but it'd be nice if these fixes were backported). Can you please re-review this PR? You may have missed that I added thorough unit tests, so I don't think there's anything else that I need to do. I see that another package has noted the existence of this PR and is also awaiting these long-standing bugs to be fixed.

@ayshih
Copy link
Contributor Author

ayshih commented Feb 25, 2023

@AA-Turner: Sorry to be a bother, but can you please re-review this? All tests are green, including the ones that I added. Thanks!

@AA-Turner
Copy link
Member

@ayshih one of the new tests is failing (test_inheritance_diagram_svg_html), please would you have a look?

A

@ayshih
Copy link
Contributor Author

ayshih commented Jul 31, 2023

@AA-Turner: So, it turns out that the recently merged #11078 addresses some of the bugs that are fixed in this PR as well, but from a different angle. This PR is specific to inheritance diagrams and generates the correctly pathed URLs before passing them to the graphviz writer, while #11078 instead modifies the graphviz writer to accept URLs with the "wrong" relative path and rewrites them to have the correct path. Since the graphviz writer has broader uses beyond just inheritance diagrams, I'll update this PR in light of #11078.

I'll note that #11078 actually seems to be slightly buggy, so I'll fix its issues. Also, this PR is not moot because the bug with intersphinx linking still needs to be addressed.

@ayshih
Copy link
Contributor Author

ayshih commented Jul 31, 2023

Okay, this PR has been updated in light of #11078, including fixing links that #11078 inadvertently broke. All tests are green! (I've also tested using a real package (SunPy), and it works as intended.)

@AA-Turner AA-Turner self-requested a review August 5, 2023 03:02
@AA-Turner AA-Turner merged commit 3e30fa3 into sphinx-doc:master Aug 8, 2023
26 checks passed
@AA-Turner
Copy link
Member

Thanks @ayshih!

A

@ayshih
Copy link
Contributor Author

ayshih commented Aug 8, 2023

Thanks!

@pllim
Copy link

pllim commented Aug 10, 2023

Hello! Which Sphinx version is this patch expected to be in? 7.1.3? 7.2.0? 8.0?

@AA-Turner
Copy link
Member

7.2.

A

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants