napari / docs

Documentation for napari (other than API docs)
BSD 3-Clause "New" or "Revised" License
9 stars 37 forks source link

Fix ALL Sphinx warnings #393

Closed melissawm closed 2 months ago

melissawm commented 3 months ago

References and relevant issues

Closes #111 Addresses #227 (will need a follow-up turning on sphinx -W option)

Description

This PR includes fixes to broken references and markdown syntax fixes. It also includes a new filter for napari.qt.threading module docstrings that were creating a few syntax warnings. These are not easily ignored by Sphinx and can't be fixed, as they are Markdown being injected into rst docstrings through the napari.qt module.

After this is merged, we will be able to make the build docs CI check to fail on new warnings, and new errors will be more visible.

psobolewskiPhD commented 2 months ago

Looking at CircleCI output: https://app.circleci.com/pipelines/github/melissawm/napari-docs/340/workflows/20e668a6-ad6f-4d21-bca7-31327ec1ed0a/jobs/328?invite=true#step-106-33254_29 Just 16 warnings! Down from >140!

Looks like a few are fixed by: https://github.com/napari/docs/pull/388 and then a few more by: https://github.com/napari/napari/pull/6812

melissawm commented 2 months ago

Look at this https://github.com/napari/docs/actions/runs/8600978497/job/23567119812?pr=393#step:9:1522 😍 Finally!

Czaki commented 2 months ago

This duplicated warning is signalig problem with cross references?

psobolewskiPhD commented 2 months ago

Interesting this barks: https://github.com/napari/docs/actions/runs/8600978497/job/23567119812?pr=393#step:9:663

WARNING: autodoc: failed to import function '_testsupport.make_napari_viewer' from module 'napari.utils'; the following exception was raised:
No module named 'pytest'

The relevant code, as far as I can tell: https://github.com/napari/docs/blob/1c6155fd6e2f9dd053559bef73bb5f04bda5878c/docs/developers/contributing/testing.md?plain=1#L243-L255 And on napari.org/dev/ it's broken: https://napari.org/dev/developers/contributing/testing.html#make-napari-viewer But with this PR it's working! https://output.circle-artifacts.com/output/job/54961a00-f23c-4027-bc52-acec2f1e3e84/artifacts/0/docs/docs/_build/html/developers/contributing/testing.html#make-napari-viewer

melissawm commented 2 months ago

This duplicated warning is signalig problem with cross references?

Not really. As far as I can tell, this happens because some Attributes and Properties have the same name, which confuses sphinx (which doesn't deal with Attributes very well). In addition, in some cases when you have subclasses that inherit the parent's docstrings this can also happen (since both objects will be documented with the same parameters/attributes). But I will confess that as much as I've investigated this, it seems like nobody really knows what's going on exactly, including me 😅 (see, for example, https://github.com/sphinx-doc/sphinx/issues/7817 which is the closest I could find)

We could potentially fix this without ignoring the errors, but that would mean having multiple templates for different classes which would make the docs generation less automatic and more manual. Since these warnings do not impact the rendering of the docs, I think they are safe to ignore.

melissawm commented 2 months ago

Interesting this barks: napari/docs/actions/runs/8600978497/job/23567119812?pr=393#step:9:663

WARNING: autodoc: failed to import function '_testsupport.make_napari_viewer' from module 'napari.utils'; the following exception was raised:
No module named 'pytest'

The relevant code, as far as I can tell:

https://github.com/napari/docs/blob/1c6155fd6e2f9dd053559bef73bb5f04bda5878c/docs/developers/contributing/testing.md?plain=1#L243-L255

And on napari.org/dev/ it's broken: napari.org/dev/developers/contributing/testing.html#make-napari-viewer But with this PR it's working! output.circle-artifacts.com/output/job/54961a00-f23c-4027-bc52-acec2f1e3e84/artifacts/0/docs/docs/_build/html/developers/contributing/testing.html#make-napari-viewer

I think we'd have to include pytest as a dependency for building the docs for this to work without this warning? Not sure why I didn't see this locally...

psobolewskiPhD commented 2 months ago

I think we'd have to include pytest as a dependency for building the docs for this to work without this warning? Not sure why I didn't see this locally...

Local build instructions use dev install of napari, so you probably have it. Edit: derp, so does circleCI workflow: https://github.com/napari/docs/blob/1c6155fd6e2f9dd053559bef73bb5f04bda5878c/.circleci/config.yml#L35

🤷

lucyleeow commented 2 months ago

is happens because some Attributes and Properties have the same name, which confuses sphinx (which doesn't deal with Attributes very well). In addition, in some cases when you have subclasses that inherit the parent's docstrings this can also happen

Maybe we could add this info in the FilterSphinxWarnings docstring?

melissawm commented 2 months ago

@lucyleeow let me know if this is enough or if I should elaborate!

psobolewskiPhD commented 2 months ago

SO I don't get the lxml warning: https://github.com/napari/docs/actions/runs/8647550683/job/23709258930?pr=393#step:9:120

I tested locally and I also get it, but yet both lxml and lxml_html_clean are installed.

Why is it referring to html-clean? it's [html_clean] in the requirements.txt which matches their setup.py It is a wierd setup.py, so maybe we should just use lxml_html_clean directly?

melissawm commented 2 months ago

I don't get this warning locally - but it looks like changing that requirement did it!

lucyleeow commented 2 months ago

ping @psobolewskiPhD could this go in? :grimacing:

psobolewskiPhD commented 2 months ago

Yeah sorry; i'm terrible at circling back to merge things.