Skip to content

Compatibility with Sphinx 8.2, minor clean up - #196

Merged
pllim merged 6 commits into
astropy:mainfrom
pllim:fix-dev-ci
Jan 10, 2025
Merged

pllim merged 6 commits into
astropy:mainfrom
pllim:fix-dev-ci

Conversation

@pllim

@pllim pllim commented Jan 10, 2025

Copy link
Copy Markdown
Member

Goal: Fix devdeps CI

I followed diff in sphinx-doc/sphinx#13212

@pllim

pllim commented Jan 10, 2025

Copy link
Copy Markdown
Member Author

wow I made it worse lol

@bsipocz

bsipocz commented Jan 10, 2025

Copy link
Copy Markdown
Member

Ahh, I started working on this just this morning, but haven't got far so will abandon the effort :)

@pllim

pllim commented Jan 10, 2025

Copy link
Copy Markdown
Member Author

Ooops sorry for the duplicated work, @bsipocz !

@pllim
pllim marked this pull request as ready for review January 10, 2025 19:32
@pllim
pllim requested a review from bsipocz January 10, 2025 19:32
@pllim

pllim commented Jan 10, 2025

Copy link
Copy Markdown
Member Author

Should use the "squash and merge" button, or if you want me to manually do it, pls lemme know. Thanks!

@codecov

codecov Bot commented Jan 10, 2025 •

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 75.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 90.09%. Comparing base (594fc12) to head (a054523).
Report is 21 commits behind head on main.

Files with missing lines Patch % Lines
sphinx_automodapi/automodsumm.py 75.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #196      +/-   ##
==========================================
- Coverage   90.29%   90.09%   -0.21%     
==========================================
  Files          30       30              
  Lines        1206     1211       +5     
==========================================
+ Hits         1089     1091       +2     
- Misses        117      120       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@bsipocz

bsipocz commented Jan 10, 2025

Copy link
Copy Markdown
Member

No worries about duplicates, I only started digging. Actually, I appreciate you opening a draft early on, so we in fact avoided doing much duplicates.

@bsipocz bsipocz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good, but maybe add a few * in the signature?

return old_generate_dot(self, name, urls=fullname_urls, env=env,
graph_attrs=graph_attrs, node_attrs=node_attrs, edge_attrs=edge_attrs)
if SPHINX_LT_8_2:
def patched_generate_dot(self, name, urls={}, env=None,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

While we're at this, if possible, make the kwargs mandatory so the API change won't have any chances to bite us back.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I added it, though hopefully won't break downstream...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@pllim

pllim commented Jan 10, 2025

Copy link
Copy Markdown
Member Author

Hmm looks like that breaks Sphinx itself... should I revert adding *?

sphinx/ext/inheritance_diagram.py", line 424, in html_visit_inheritance_diagram
    dotcode = graph.generate_dot(name, urls, env=self.builder.env)

@bsipocz

bsipocz commented Jan 10, 2025

Copy link
Copy Markdown
Member

Ahh, it was worth a try. Go ahead and merge it once CI passes.

@pllim
pllim merged commit 77a0e29 into astropy:main Jan 10, 2025
@pllim

pllim commented Jan 10, 2025

Copy link
Copy Markdown
Member Author

Thanks for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants