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 svg_link_prefix option to inheritance_diagram #6454

Closed

Conversation

eteq
Copy link

@eteq eteq commented Jun 6, 2019

Subject: Add an option to set the prefix for links in SVG inheritance diagrams

Feature or Bugfix

  • Feature

Purpose

  • Provide a way for extensions or projects that embed the inheritance diagram SVG in pages to have the links work.

Detail

As discussed in astropy/astropy#4935 and #2484, the inheritance diagram machinery currently yields broken links as we implemented it in sphinx-automodapi and astropy. You can see an example here: https://docs.astropy.org/en/v3.1.2/coordinates/index.html#class-inheritance-diagram . The gist of the problem (as discussed in #2484) is that we embed the SVG in the page. This means the path to the actual class pages don't have the '..' that apparently gets created when embedding as a separate file. (FWIW, the links originally worked when we first did this, but then the changes in #967 ended up breaking it. And here we are years later with me finally having realized the underlying cause!).

So this PR introduces a new config option in inheritance_diagram that sets the prefix. The default is still ../, so it shouldn't break other code, but it will then be possible to supress the .. in cases like ours where it's breaking the links. I think it may partly address #3176 by giving that use case the flexibility to pick their own prefix as well (although I'm not 100% sure about that one).

Relates

@eteq
Copy link
Author

eteq commented Jun 6, 2019

Also, I wasn't entirely sure if this was supposed to be against the 2.1.1 or the master branch - I'm not clear on whether adding new configuration items is considered backwards-compatibility breaking or not, so using master seemed like the best choice. I can re-submit it against the 2.1.1 branch if I mis-interpreted that, though.

@codecov
Copy link

codecov bot commented Jun 6, 2019

Codecov Report

Merging #6454 into master will decrease coverage by <.01%.
The diff coverage is 60%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6454      +/-   ##
==========================================
- Coverage   84.22%   84.22%   -0.01%     
==========================================
  Files         280      277       -3     
  Lines       39942    39890      -52     
  Branches     5899     5897       -2     
==========================================
- Hits        33643    33597      -46     
+ Misses       4989     4985       -4     
+ Partials     1310     1308       -2
Impacted Files Coverage Δ
sphinx/ext/inheritance_diagram.py 81.69% <60%> (+0.16%) ⬆️
sphinx/writers/__init__.py
tests/roots/test-setup/setup.py
tests/roots/test-setup/doc/conf.py

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5ce5c2c...536b17b. Read the comment docs.

@tk0miya
Copy link
Member

tk0miya commented Jun 8, 2019

What case are users need to change this? If my understanding correct, this is only used for automodapi extension. If so, I propose you to modify node['refuri'] for SVG images on automodapi side.
Thanks,

@tk0miya tk0miya added extensions type:proposal a feature suggestion labels Jun 8, 2019
@tk0miya
Copy link
Member

tk0miya commented Jun 30, 2019

I'm closing this because main usecase of this PR is not clear.
Please feel free to reopen if you'd like to merge this again.
Thanks,

@tk0miya tk0miya closed this Jun 30, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 11, 2021
@AA-Turner AA-Turner added extensions:graphviz The `sphinx.ext.graphviz` or `sphinx.ext.inheritance_diagram` extensions and removed extensions labels Jan 29, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
closed:wontfix extensions:graphviz The `sphinx.ext.graphviz` or `sphinx.ext.inheritance_diagram` extensions type:proposal a feature suggestion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants