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

Fix broken inheritance-diagram links due to the smart resolver #172

Merged
merged 2 commits into from
Aug 10, 2023

Conversation

ayshih
Copy link
Contributor

@ayshih ayshih commented Aug 9, 2023

sphinx-doc/sphinx#10614 fixes many of the broken inheritance-diagram links (#48). The remaining broken links are due to a bad interaction with sphinx-automodapi's smart resolver.

sphinx.ext.inheritance_diagram generates a list of class full names (e.g., astropy.coordinates.baseframe.BaseCoordinateFrame) and generates a mapping from class full names to documentation URLs. However, the smart resolver causes the generated mapping to be instead from class documented name (e.g., astropy.coordinates.BaseCoordinateFrame) to documentation URLs. The class documented name can be different from the class full name if the class is not documented where it is defined, but rather at some other location where it is imported. In such situations, the code will fail to find the URL that for the class.

This PR monkey-patches the method that receives the mapping and converts the keys from class documented names to class full names.

Also address #173 (but not the part about updating matrix)

@codecov
Copy link

codecov bot commented Aug 10, 2023

Codecov Report

Merging #172 (508fab4) into main (b389ee2) will increase coverage by 0.04%.
The diff coverage is 100.00%.

❗ Current head 508fab4 differs from pull request most recent head 5d6ab81. Consider uploading reports for the commit 5d6ab81 to get more accurate results

@@            Coverage Diff             @@
##             main     #172      +/-   ##
==========================================
+ Coverage   89.83%   89.87%   +0.04%     
==========================================
  Files          27       27              
  Lines        1141     1146       +5     
==========================================
+ Hits         1025     1030       +5     
  Misses        116      116              
Files Changed Coverage Δ
sphinx_automodapi/automodsumm.py 85.92% <100.00%> (+0.21%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@ayshih
Copy link
Contributor Author

ayshih commented Aug 10, 2023

This PR also fixes a bug that results from the fact that newer versions of Sphinx use pathlib for paths rather than just strings.

@ayshih ayshih marked this pull request as ready for review August 10, 2023 01:00
@pllim
Copy link
Member

pllim commented Aug 10, 2023

Thanks! Didn't you have a draft PR on astropy to test this? Or was that for something else? I don't see it linked here.

@ayshih
Copy link
Contributor Author

ayshih commented Aug 10, 2023

Sure. I've repurposed astropy/astropy#15104 to test against sphinx dev (not my fork) and my sphinx-automodapi fork

@pllim
Copy link
Member

pllim commented Aug 10, 2023

Sigh, I guess it is unfair to you if pydata/pydata-sphinx-theme#1404 blocks this. I guess if you have another draft PR to Sunpy or something that can show the diagram is fixed, that would be also acceptable.

@ayshih
Copy link
Contributor Author

ayshih commented Aug 10, 2023

Confirmed to work for sunpy (sunpy/sunpy#6267 (comment)), although we'd be blocked from using it immediately because our normal theme pins us to Sphinx 5.3. Without sphinx-doc/sphinx#10614, this PR has no effect.

Copy link
Member

@pllim pllim left a comment

Choose a reason for hiding this comment

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

Thanks for all your work and patience in fixing this long standing bug!

I guess the upstream fix will be some release after sphinx 7.1.2. We can make a release here once they release, so feel free to ping us again if we are too slow.

😸

@pllim pllim merged commit 4de617e into astropy:main Aug 10, 2023
18 checks passed
@pllim pllim mentioned this pull request Aug 10, 2023
@pllim pllim added this to the 0.16.0 milestone Aug 10, 2023
@bsipocz
Copy link
Member

bsipocz commented Aug 17, 2023

I guess the upstream fix will be some release after sphinx 7.1.2

Would you release, or shall I?

@pllim
Copy link
Member

pllim commented Aug 17, 2023

@bsipocz , I need to take a lunch break now, so if you can do it faster, please go ahead!

@bsipocz
Copy link
Member

bsipocz commented Aug 17, 2023

I need to take a lunch break now, so if you can do it faster, please go ahead!

not faster, but I can do it in the evening.

@pllim
Copy link
Member

pllim commented Aug 17, 2023

In that case, let me just do it. I am back from break. Thanks for the ping!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants