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

📚⬆️ UPDATE: sphinx v4 (+ sphinx extensions) #5420

Merged
merged 2 commits into from
Mar 9, 2022

Conversation

chrisjsewell
Copy link
Member

@chrisjsewell chrisjsewell commented Mar 7, 2022

This PR updates sphinx to version 4, as well as updating docutils, pygments, pydata-sphinx-theme, and sphinx-copybutton.

A key issue for this upgrade, was that sphinx.ext.autodoc reads classes/functions from both modules and also from __all__.
In sphinx 3 this was actually failing silently, by duplicating all the API documentation for these objects, for example:

In sphinx 4, this now emits a duplicate object description warning.

To fix this, the ignore-module-all flag has been added to all autodoc directives.
Then, to overcome the fact that we would still like to refer to the top-level module paths (e.g. aiida.orm.Computer rather than aiida.orm.computers.Computer), some code has been added in conf.py, to specify these aliases.

Note, I think long-term, it might just be better to remove the use of sphinx-apidoc, and specify the modules to document explicitly, using only their __all__, i.e. only create API documentation for public APIs.
This is a bigger change though (plus non-essential) so I'd push that back to a later date.

You'll also see that most of the nitpick-exceptions have now been removed 😄

@chrisjsewell chrisjsewell changed the title Update sphinx v4 ⬆️ UPDATE: sphinx v4 (+ sphinx extensions) Mar 7, 2022
@chrisjsewell chrisjsewell requested review from sphuber and csadorf March 7, 2022 23:02
@chrisjsewell chrisjsewell changed the title ⬆️ UPDATE: sphinx v4 (+ sphinx extensions) 📚⬆️ UPDATE: sphinx v4 (+ sphinx extensions) Mar 7, 2022
@chrisjsewell
Copy link
Member Author

Note, this also removes the need to pin markupsafe, since sphinx 4.1 does not pin jinja2: sphinx-doc/sphinx@85f5887

Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

Just two minor comments

Comment on lines 191 to 192
# ignore anything ending in `Type`, since it is likely a `typing.TypeVar`, which sphinx-apidoc doesn't handle
nitpick_ignore_regex = [('py:.*', '.+Type')]
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, this also includes all our custom ParameterType implementations in aiida.cmdline.params.types. Don't think it is the worst, but is there a way we can exclude those?

Copy link
Member Author

Choose a reason for hiding this comment

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

This just stops a warning from being emitted if it can't resolve the reference, it won't stop the reference from being resolved if present

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I get that, but if there is warning related to our param types, that is probably because there is a real problem, and this way it will go unnoticed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Eurgh, unless you are literally going to be using :py:obj:`aiida.cmdline.params.types.WrongType` in the documentation, or a docstring, I don't think it's going to be a big problem. Obviously, actual typing errors should be tested by mypy

Copy link
Contributor

Choose a reason for hiding this comment

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

But that is the whole point of these warnings, to warn you if you (by accident) type a wrong resource, for example a typo.

Copy link
Member Author

Choose a reason for hiding this comment

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

There are literally no sphinx references to ParamType anywhere in the documentation/docstrings, but anyhow I've removed this and reinstated a lovely long nitpick_exceptions list.

@@ -126,7 +123,8 @@ tests = [
"pympler~=0.9",
"coverage<5.0",
"sqlalchemy-utils~=0.37.2",
"sphinx~=3.2.1"
"sphinx~=4.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to have ~=4.0 here but ~=4.1 in the docs extra?

Copy link
Member Author

Choose a reason for hiding this comment

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

nitpick_ignore_regex was only introduced in 4.1, added for our docs build, but for testing the aiida sphinx extension, it is only sphinx 4 that is needed (it still actually works for sphinx 3, but there is a change in the AST, when regression testing the output)

docs/source/conf.py Outdated Show resolved Hide resolved
@sphuber sphuber self-requested a review March 9, 2022 08:02
@chrisjsewell chrisjsewell merged commit 58a5d80 into aiidateam:develop Mar 9, 2022
@chrisjsewell chrisjsewell deleted the update-sphinx branch March 9, 2022 08:05
@csadorf csadorf added the dependencies Pull requests that update a dependency file label Mar 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dependencies: update Sphinx requirement Docs: API docs cumbersome to browse
3 participants