Skip to content
This repository has been archived by the owner on Oct 14, 2023. It is now read-only.

Build API docs with sphinx-autoapi #1000

Merged
merged 5 commits into from
Aug 2, 2020

Conversation

jorgepiloto
Copy link
Member

@jorgepiloto jorgepiloto commented Jul 22, 2020

Solves for #967 by making use of sphinx-autoapi for building project's API documentation. Although very useful, some warnings were raise, which is treated as an error by tox -e docs. Some of this warnings are related with typos and therefore, easy to fix. However, others are related with imports and I am not sure why sphinx-autoapi is not able to properly parse those poliastro objects.

I deleted the whole docs/source/api directory since it only contained *.rst files with the minimum required layout for building pages documentation, that is the reason behind so many simplified lines of code. By default sphinx-autoapi builds a temporary directory called autoapi inside docs/source and generates all necessary *.rstfiles. After desired output files (*.html, *.tex, etc...) are built, the autoapi folder is deleted.

@codecov
Copy link

codecov bot commented Jul 22, 2020

Codecov Report

Merging #1000 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1000      +/-   ##
==========================================
+ Coverage   89.49%   89.52%   +0.02%     
==========================================
  Files          74       74              
  Lines        3932     3941       +9     
  Branches      343      343              
==========================================
+ Hits         3519     3528       +9     
  Misses        322      322              
  Partials       91       91              
Impacted Files Coverage Δ
src/poliastro/core/propagation/__init__.py 94.01% <ø> (ø)
src/poliastro/czml/extract_czml.py 88.09% <ø> (ø)
src/poliastro/earth/__init__.py 100.00% <ø> (ø)
src/poliastro/earth/atmosphere/base.py 86.84% <ø> (ø)
src/poliastro/spacecraft/__init__.py 100.00% <ø> (ø)
src/poliastro/twobody/orbit.py 87.05% <ø> (ø)
src/poliastro/constants/general.py 100.00% <100.00%> (ø)
src/poliastro/frames/ecliptic.py 100.00% <100.00%> (ø)
src/poliastro/frames/equatorial.py 82.75% <100.00%> (+0.94%) ⬆️
src/poliastro/frames/fixed.py 89.22% <100.00%> (+0.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8ae3e03...24b85dd. Read the comment docs.

@astrojuanlu
Copy link
Member

Thanks a lot for this @jorgepiloto! Let me have a look locally

@jorgepiloto
Copy link
Member Author

Some warnings might be the following:

In poliastro.constants.general.py, some planets mass are taken directly from the astropy package as seen in the following permalink:

from astropy.constants.iau2015 import M_earth, M_jup as M_jupiter, M_sun

Then, they are added to the __all__ attribute of the module:

"M_earth",
"M_jupiter",
"M_sun",

I think this is causing the error, since they are not directly defined within the source code. The sphinx-autopi tries to look for astropy/ in the src/ directory to retrieve proper definition but, of course, it is not able to find the package because of is dependency nature.

@astrojuanlu
Copy link
Member

In the CI we have sphinx.errors.SphinxWarning: Cannot resolve import of poliastro.frames.equatorial.GCRS in poliastro.frames.ecliptic, and locally I see sphinx.errors.SphinxWarning: Cannot resolve import of poliastro.constants.general.M_earth in poliastro.constants. 🤔

@jorgepiloto
Copy link
Member Author

From official documentation:

Anything that is imported into a module is not documented. Usually a module is where implementations exist. Therefore an import of something is usually for the usage of the implementation, and not as something to be accessed publicly.

@jorgepiloto
Copy link
Member Author

I found a little hack. If the following line:

from astropy.constants.iau2015 import M_earth, M_jup as M_jupiter, M_sun

is substituted by:

from astropy.constants.iau2015 import M_earth as _M_earth, M_jup as _M_jupiter, M_sun as _M_sun

# Custom variable definition
M_earth = _M_earth
M_jupiter = _M_jupiter
M_sun = _M_sun

Autoapi does not complain, since these vars have been defined in the module.

@astrojuanlu
Copy link
Member

astrojuanlu commented Jul 22, 2020 via email

@jorgepiloto
Copy link
Member Author

Although the cannot import... warnings were fixed with previous hack, there are still some others related to duplicated docstrings or definitions... 😓

@astrojuanlu
Copy link
Member

Beware of typos 😉

@astrojuanlu
Copy link
Member

I showed this PR to @humitos who helped me a lot understand what was going on (thanks!).

For the sphinx.errors.SphinxWarning: Cannot resolve import of ... warnings, it's likely because sphinx-autoapi performs static analysis to extract the documentation, and those variables are not defined there.

Options:

  1. The hack that @jorgepiloto implemented (ugh 😇)
  2. Excluding the problematic modules (using "exclude patterns") and document those manually
  3. Try a different solution, for example https://github.com/sphinx-contrib/apidoc (see discussion about sphinx-apidoc on Read the Docs at Support auto-generated API docs readthedocs/readthedocs.org#1139 and Add how to use apidoc guide/doc to our user docs readthedocs/readthedocs.org#7162)

I think option (2) is the best, given that we already started the effort of using sphinx-apidoc. We can always resort to (3) if all else fails.

Apart from that, I see that we're getting several warnings:

/home/docs/checkouts/readthedocs.org/user_builds/poliastro/checkouts/1000/docs/source/autoapi/poliastro/core/propagation/index.rst:35: WARNING: duplicate object description of poliastro.core.propagation.farnocchia, other instance in autoapi/poliastro/core/propagation/farnocchia/index, use :noindex: for one of them
/home/docs/checkouts/readthedocs.org/user_builds/poliastro/checkouts/1000/docs/source/autoapi/poliastro/czml/extract_czml/index.rst:69: WARNING: Unexpected indentation.
/home/docs/checkouts/readthedocs.org/user_builds/poliastro/checkouts/1000/docs/source/autoapi/poliastro/czml/extract_czml/index.rst:70: WARNING: Block quote ends without a blank line; unexpected unindent.
/home/docs/checkouts/readthedocs.org/user_builds/poliastro/checkouts/1000/docs/source/autoapi/poliastro/earth/atmosphere/base/index.rst:59: WARNING: Unexpected section title.

Paramters
---------
/home/docs/checkouts/readthedocs.org/user_builds/poliastro/checkouts/1000/docs/source/autoapi/poliastro/earth/index.rst:166: WARNING: Unexpected section title.

Returns
-------
/home/docs/checkouts/readthedocs.org/user_builds/poliastro/checkouts/1000/docs/source/autoapi/poliastro/earth/index.rst:171: WARNING: Unexpected section title.

Notes
-----
/home/docs/checkouts/readthedocs.org/user_builds/poliastro/checkouts/1000/docs/source/autoapi/poliastro/earth/index.rst:175: WARNING: Literal block expected; none found.
/home/docs/checkouts/readthedocs.org/user_builds/poliastro/checkouts/1000/docs/source/autoapi/poliastro/spacecraft/index.rst:51: WARNING: Unexpected section title.

Returns
-------
/home/docs/checkouts/readthedocs.org/user_builds/poliastro/checkouts/1000/docs/source/autoapi/poliastro/spacecraft/index.rst:56: WARNING: Unexpected section title.

Notes
-----
/home/docs/checkouts/readthedocs.org/user_builds/poliastro/checkouts/1000/docs/source/autoapi/poliastro/spacecraft/index.rst:60: WARNING: Literal block expected; none found.
/home/docs/checkouts/readthedocs.org/user_builds/poliastro/checkouts/1000/docs/source/autoapi/poliastro/twobody/thrust/index.rst:34: WARNING: duplicate object description of poliastro.twobody.thrust.change_a_inc, other instance in autoapi/poliastro/twobody/thrust/change_a_inc/index, use :noindex: for one of them
/home/docs/checkouts/readthedocs.org/user_builds/poliastro/checkouts/1000/docs/source/autoapi/poliastro/twobody/thrust/index.rst:64: WARNING: duplicate object description of poliastro.twobody.thrust.change_argp, other instance in autoapi/poliastro/twobody/thrust/change_argp/index, use :noindex: for one of them
/home/docs/checkouts/readthedocs.org/user_builds/poliastro/checkouts/1000/docs/source/autoapi/poliastro/twobody/thrust/index.rst:74: WARNING: duplicate object description of poliastro.twobody.thrust.change_ecc_quasioptimal, other instance in autoapi/poliastro/twobody/thrust/change_ecc_quasioptimal/index, use :noindex: for one of them
/home/docs/checkouts/readthedocs.org/user_builds/poliastro/checkouts/1000/docs/source/autoapi/poliastro/twobody/thrust/index.rst:88: WARNING: duplicate object description of poliastro.twobody.thrust.change_inc_ecc, other instance in autoapi/poliastro/twobody/thrust/change_inc_ecc/index, use :noindex: for one of them

@jorgepiloto
Copy link
Member Author

I also noticed is possible to generate docstrings for constants by doing:

UNITY = 1.00
""" Unitary constant """

Might trigger the sphinx-autopi and, therefore, get rid of some warnings.

@jorgepiloto
Copy link
Member Author

I was able locally to get ride of some warnings and end up having only those ones claiming for the addition of :noindex and more than one target found.... Still working on this and taking a look to previously reported suggestions 🚀

@jorgepiloto
Copy link
Member Author

OMG: I solved it in an easy way! Let me rewrite git history and push changes 👍

@astrojuanlu
Copy link
Member

Wohoo amazing! Looking forward to the finished version :)

Copy link
Member

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

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

The result is excellent! Lots of undocumented modules and packages appeared, we were forced to add docstrings to many of them (which is a good thing) and the organization is good. I think this is a great improvement over what we had before and you executed beautifully, so I am merging this. Thanks a lot @jorgepiloto!

@astrojuanlu astrojuanlu merged commit c78984e into poliastro:master Aug 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants