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 option to set :noindex: #150

Merged
merged 9 commits into from Dec 9, 2022
Merged

Add option to set :noindex: #150

merged 9 commits into from Dec 9, 2022

Conversation

saimn
Copy link
Contributor

@saimn saimn commented Apr 20, 2022

Fix #130.

TODO: see if I can add a test for this.

@saimn saimn added this to the 0.15.0 milestone Apr 20, 2022
@codecov
Copy link

codecov bot commented Apr 20, 2022

Codecov Report

Merging #150 (c029e0e) into main (c4aa837) will increase coverage by 0.59%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #150      +/-   ##
==========================================
+ Coverage   89.31%   89.90%   +0.59%     
==========================================
  Files          24       27       +3     
  Lines        1104     1139      +35     
==========================================
+ Hits          986     1024      +38     
+ Misses        118      115       -3     
Impacted Files Coverage Δ
sphinx_automodapi/automodapi.py 93.93% <100.00%> (+0.18%) ⬆️
sphinx_automodapi/automodsumm.py 85.71% <100.00%> (+0.08%) ⬆️
...pi/tests/duplicated_warning/duplicated/__init__.py 100.00% <100.00%> (ø)
...ests/duplicated_warning/duplicated/foo/__init__.py 100.00% <100.00%> (ø)
...api/tests/duplicated_warning/duplicated/foo/foo.py 100.00% <100.00%> (ø)
...utomodapi/tests/example_module/abstract_classes.py 64.28% <100.00%> (+5.46%) ⬆️
sphinx_automodapi/tests/test_cases.py 100.00% <100.00%> (ø)
sphinx_automodapi/utils.py 91.30% <100.00%> (+0.47%) ⬆️
sphinx_automodapi/autodoc_enhancements.py 100.00% <0.00%> (+4.76%) ⬆️

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

@saimn
Copy link
Contributor Author

saimn commented May 6, 2022

Wasn't easy but I managed to add a test :)
Who wants to review ?

@saimn

This comment was marked as resolved.

@saimn saimn force-pushed the noindex branch 2 times, most recently from 31365f9 to 3fed892 Compare May 6, 2022 20:33
@saimn
Copy link
Contributor Author

saimn commented May 6, 2022

All green now :)

@saimn saimn requested a review from a team May 6, 2022 20:55
@pllim
Copy link
Member

pllim commented May 6, 2022

Ooooo... how do we use this in astropy? Does it just magically work?

@saimn
Copy link
Contributor Author

saimn commented May 6, 2022

No magic :D, we must add :noindex: to the duplicated directives, and this PR just propagates :noindex: to autodoc.

@pllim
Copy link
Member

pllim commented May 9, 2022

@saimn , is there an astropy PR that uses this feature? Would be nice to see if it really works before we merge.

@saimn
Copy link
Contributor Author

saimn commented May 19, 2022

is there an astropy PR that uses this feature? Would be nice to see if it really works before we merge.

@pllim - no, I could do that indeed 👍

@pllim
Copy link
Member

pllim commented Aug 11, 2022

@saimn , any chance we can push this forward? Did you say there is something that not quite working still using this with astropy?

@WilliamJamieson is unable to build astropy docs on Python 3.10 because of the Sphinx maxversion pin.

@saimn
Copy link
Contributor Author

saimn commented Dec 2, 2022

Within another round of testing/debugging on Astropy I'm pretty convinced that the changes here are correct and are doing what is supposed. Adding :noindex: should help fixing duplicate warning for simple cases such as https://github.com/maxnoe/sphinx-automodapi-duplicated-warning, and probably photutils. But for astropy it's not enough because of the complicated structure of the docs and code, using :noindex: there then creates some other warnings about missing references.

@pllim
Copy link
Member

pllim commented Dec 2, 2022

Is it time to make astropy less complicated?! We cannot pin Sphinx forever.

@Cadair
Copy link
Member

Cadair commented Dec 7, 2022

Is it time to make astropy less complicated?!

I have been working on this for years 😛

Copy link
Member

@Cadair Cadair left a comment

Choose a reason for hiding this comment

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

:shipit:

@@ -0,0 +1,11 @@
project = 'duplicated'
copyright = '2022, Maximilian Nöthe'
Copy link
Member

Choose a reason for hiding this comment

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

LoL does @maxnoe consent to be immortalized as a test case like this? 😸

Copy link
Member

Choose a reason for hiding this comment

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

sure

Copy link
Member

Choose a reason for hiding this comment

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

Though I am Maximilian Linhoff now ;)

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.

Just some nitpicks. If you want to address them, great. If not, we can defer. Though that one typo should probably be fixed anyway before merge.

Let's ship this indeed and then fix up astropy! 🚀

@pllim pllim mentioned this pull request Dec 9, 2022
10 tasks
@Cadair
Copy link
Member

Cadair commented Dec 9, 2022

Even though astropy core doesn't need this, we might as well merge it?

@pllim

This comment was marked as resolved.

@pllim
Copy link
Member

pllim commented Dec 9, 2022

Gah, CI got disabled. I have to close/reopen.

@pllim pllim closed this Dec 9, 2022
@pllim pllim reopened this Dec 9, 2022
@pllim pllim merged commit 6e476f2 into astropy:main Dec 9, 2022
@pllim
Copy link
Member

pllim commented Dec 9, 2022

Merged! I'll let someone else do the release. Thanks, all!

@saimn saimn deleted the noindex branch December 9, 2022 18:22
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.

Incompatible with sphinx>4 (submodules of submodules)
5 participants