Skip to content

Add option to set :noindex: - #150

Merged
pllim merged 9 commits into
astropy:mainfrom
saimn:noindex
Dec 9, 2022
Merged

Add option to set :noindex:#150
pllim merged 9 commits into
astropy:mainfrom
saimn:noindex

Conversation

@saimn

@saimn saimn commented Apr 20, 2022

Copy link
Copy Markdown
Contributor

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

codecov Bot commented Apr 20, 2022 •
edited
Loading

Copy link
Copy Markdown

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

saimn commented May 6, 2022

Copy link
Copy Markdown
Contributor Author

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

@saimn

This comment was marked as resolved.

Sign in to view
@saimn
saimn force-pushed the noindex branch 2 times, most recently from 31365f9 to 3fed892 Compare May 6, 2022 20:33
@saimn

saimn commented May 6, 2022

Copy link
Copy Markdown
Contributor Author

All green now :)

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

pllim commented May 6, 2022

Copy link
Copy Markdown
Member

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

@saimn

saimn commented May 6, 2022

Copy link
Copy Markdown
Contributor Author

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

@pllim

pllim commented May 9, 2022

Copy link
Copy Markdown
Member

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

astrofrog
astrofrog reviewed May 19, 2022
View reviewed changes
Comment thread sphinx_automodapi/tests/test_cases.py
@saimn

saimn commented May 19, 2022

Copy link
Copy Markdown
Contributor Author

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 pllim mentioned this pull request Jun 20, 2022
Closed
2 tasks
@pllim

pllim commented Aug 11, 2022

Copy link
Copy Markdown
Member

@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.

@pllim pllim mentioned this pull request Aug 30, 2022
Closed
@saimn saimn mentioned this pull request Dec 2, 2022
Closed
@saimn

saimn commented Dec 2, 2022

Copy link
Copy Markdown
Contributor Author

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

pllim commented Dec 2, 2022

Copy link
Copy Markdown
Member

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

@Cadair

Cadair commented Dec 7, 2022

Copy link
Copy Markdown
Member

Is it time to make astropy less complicated?!

I have been working on this for years 😛

Cadair
Cadair approved these changes Dec 7, 2022
View reviewed changes

@Cadair Cadair 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.

:shipit:

pllim
pllim reviewed Dec 7, 2022
View reviewed changes
Comment thread sphinx_automodapi/tests/duplicated_warning/docs/conf.py Outdated
@@ -0,0 +1,11 @@
project = 'duplicated'
copyright = '2022, Maximilian Nöthe'

@pllim pllim Dec 7, 2022

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.

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

@maxnoe maxnoe Dec 7, 2022

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.

sure

@maxnoe maxnoe Dec 7, 2022

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.

Though I am Maximilian Linhoff now ;)

pllim
pllim reviewed Dec 7, 2022
View reviewed changes
Comment thread sphinx_automodapi/tests/duplicated_warning/duplicated/foo/foo.py Outdated
pllim
pllim approved these changes Dec 7, 2022
View reviewed changes

@pllim pllim 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.

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 reviewed Dec 7, 2022
View reviewed changes
Comment thread sphinx_automodapi/tests/duplicated_warning/docs/conf.py Outdated
Comment thread sphinx_automodapi/tests/duplicated_warning/docs/conf.py Outdated
@pllim pllim mentioned this pull request Dec 9, 2022
Merged
8 of 10 tasks
@Cadair

Cadair commented Dec 9, 2022

Copy link
Copy Markdown
Member

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

@pllim

This comment was marked as resolved.

Sign in to view
@pllim

pllim commented Dec 9, 2022

Copy link
Copy Markdown
Member

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

pllim commented Dec 9, 2022

Copy link
Copy Markdown
Member

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

@saimn
saimn deleted the noindex branch December 9, 2022 18:22
@pllim pllim mentioned this pull request Apr 7, 2023
Open
This was referenced Apr 7, 2025
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@astrofrog astrofrog astrofrog left review comments
@pllim pllim pllim approved these changes
@Cadair Cadair Cadair approved these changes
+1 more reviewer
@maxnoe maxnoe maxnoe left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.15.0

Development

Successfully merging this pull request may close these issues.

Incompatible with sphinx>4 (submodules of submodules)

5 participants

@saimn @pllim @Cadair @astrofrog @maxnoe