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

#8 Add Sphinx Autodoc #61

Merged
merged 3 commits into from Aug 26, 2017

Conversation

Projects
None yet
4 participants
@WGierke
Copy link
Contributor

WGierke commented Aug 21, 2017

Description

This pull request should add the ability to auto-generate code documentation based on Google-styled docstrings.

Reference to official issue

This should solve #8

Motivation and Context

Instead of manually adapting the documentation to the written code and its docstrings, we can now auto-generate the Sphinx documentation based on the docstrings in the code.

How Has This Been Tested?

  1. I merged my code into #55 (so it's easier for me to update the documentation :-) )
  2. I started docker-compose
  3. I saw the auto-summaries of all modules, classes and methods that have proper docstrings
  4. To confirm that it really supports Google-style docstrings, I downloaded this sample code having lots of Google-docstrings
  5. I restarted docker-compose
  6. I confirmed that the docstrings of that file were added to the Sphinx documentation

Screenshots (if appropriate):

Overview
screenshot from 2017-08-22 01-12-20
Details of the interface.backend package
screenshot from 2017-08-22 01-12-35
One legacy method that already has a complete docstring
screenshot from 2017-08-22 01-13-10
The Google-docstring file I added for testing purposes was rendered correctly
screenshot from 2017-08-22 01-04-16

CLA

  • I have signed the CLA; if other committers are in the commit history, they have signed the CLA as well

@WGierke WGierke force-pushed the WGierke:8_sphinx_autodoc branch from 011efdd to 8fc4b8b Aug 22, 2017

@isms

This comment has been minimized.

Copy link
Contributor

isms commented Aug 22, 2017

@WGierke FWIW, while we diagnose #63 your build is failing on this line from test_docker.sh:

$ sh tests/test_docker.sh
+docker-compose -f local.yml run prediction pytest
Creating network "concepttoclinic_default" with the default driver
Creating volume "concepttoclinic_postgres_data_dev" with default driver
============================= test session starts ==============================
platform linux -- Python 3.6.2, pytest-3.1.3, py-1.4.34, pluggy-0.4.0
rootdir: /app, inifile:
collected 4 items / 1 errors 
==================================== ERRORS ====================================
_________________ ERROR collecting src/tests/test_endpoints.py _________________
ImportError while importing test module '/app/src/tests/test_endpoints.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
src/tests/test_endpoints.py:13: in <module>
    from src.factory import create_app
E   ModuleNotFoundError: No module named 'src'
!!!!!!!!!!!!!!!!!!! Interrupted: 1 errors during collection !!!!!!!!!!!!!!!!!!!!
=========================== 1 error in 0.72 seconds ============================
The command "sh tests/test_docker.sh" exited with 2.
@WGierke

This comment has been minimized.

Copy link
Contributor

WGierke commented Aug 22, 2017

Thanks a lot @isms that clearly helps :)

@isms

This comment has been minimized.

Copy link
Contributor

isms commented Aug 22, 2017

@WGierke This may be a little less painful if you can run the commands in test_docker.sh locally - here's the broken part.

$ flake8 prediction
prediction/src/tests/test_endpoints.py:10:1: E402 module level import not at top of file
prediction/src/tests/test_endpoints.py:11:1: E402 module level import not at top of file
prediction/src/tests/test_endpoints.py:12:1: E402 module level import not at top of file
prediction/src/tests/test_endpoints.py:14:1: E402 module level import not at top of file
prediction/src/tests/test_endpoints.py:15:1: E402 module level import not at top of file
@WGierke

This comment has been minimized.

Copy link
Contributor

WGierke commented Aug 22, 2017

Thanks a lot @isms , I was able to fix everything :)

from src.factory import create_app
import sys
sys.path.append('.')
from src.factory import create_app # NOQA

This comment has been minimized.

@pjbull

pjbull Aug 22, 2017

Contributor

Hm, it's strange for this test to begin failing with this change.

It's probably because the change adds __init__.pys in prediction and interface. Two things to try instead of changing sys.path:
(1) Does doing a relative import work? (e.g. from .factory)
(2) Does importing prediction.src.factory work?

Other than that, we can also look at the call to pytest. IIRC there are a couple ways to do that which change how imports work for the test cases.

This comment has been minimized.

@WGierke

WGierke Aug 22, 2017

Contributor

Hey @pjbull thanks for your feedback!
I tried out your suggestions but that leads to

  1. ModuleNotFoundError: No module named 'app.src.tests.factory'
  2. ModuleNotFoundError: No module named 'prediction'

I'm not that familiar with pytest but I can do some research about that.

This comment has been minimized.

@pjbull

pjbull Aug 22, 2017

Contributor

Thanks, also just to confirm, do the tests pass locally if you remove the __init__.py files?

This comment has been minimized.

@WGierke

WGierke Aug 22, 2017

Contributor

Removing interface/__init__.py and prediction/__init__.py makes the tests still pass but Sphinx unable to detect interface and prediction as package so it won't be able to automatically summarize the classes using their docstrings.

This comment has been minimized.

@isms

isms Aug 22, 2017

Contributor

What about just tweaking the Makefile to add a step touch [whatever]/__init__.py then making the docs then removing them?

This comment has been minimized.

@WGierke

WGierke Aug 24, 2017

Contributor

Very good idea, thanks @isms ! That should implement it this way

@WGierke WGierke force-pushed the WGierke:8_sphinx_autodoc branch 2 times, most recently from ea443bc to 2605ea3 Aug 22, 2017

@isms

This comment has been minimized.

Copy link
Contributor

isms commented Aug 22, 2017

@WGierke If you're making a series of commits, would you mind pushing only once you've finished a chunk of work? The flurry of small pushed commits are jamming up the Travis build queue!

Also don't forget to rebase onto master one you're ready for a review 👍

@WGierke

This comment has been minimized.

Copy link
Contributor

WGierke commented Aug 22, 2017

@isms oh I'm sorry. I wasn't able to see that as I have no access to the Travis site. Are you able to cancel them?
Would another possibility be to squash the commits when merging the branch?

@lamby

This comment has been minimized.

Copy link
Contributor

lamby commented Aug 24, 2017

I wasn't able to see that as I have no access to the Travis site.

This has been filed as #68

@WGierke WGierke force-pushed the WGierke:8_sphinx_autodoc branch 3 times, most recently from 38f7631 to 22a1e5f Aug 24, 2017

html:
# Make interface and prediction to packages so apidoc can detect them
touch interface/__init__.py

This comment has been minimized.

@lamby

lamby Aug 25, 2017

Contributor

Problem here is that if we fail to complete the build, we leave these files lying around in the repo. This is both messy, but also a problem in that even if we .gitignored them, as they are now packages this is going surely going to cause some grief wrt tests or.. something. Anyway, gut feel is that this is wrong. :/

@@ -0,0 +1,61 @@
backend\.api package

This comment has been minimized.

@lamby

lamby Aug 25, 2017

Contributor

How come we need to "escape" the dots?

@WGierke WGierke force-pushed the WGierke:8_sphinx_autodoc branch from 22a1e5f to f25b9ad Aug 25, 2017

@WGierke

This comment has been minimized.

Copy link
Contributor

WGierke commented Aug 25, 2017

@lamby Sorry for the delay, I finally figured out how to solve it. Now sphinx-apidoc should be used correctly and the paths in conf.py should also be fixed.
screenshot from 2017-08-25 19-17-50
screenshot from 2017-08-25 19-18-17

@@ -7,6 +7,10 @@ SPHINXBUILD = sphinx-build
SPHINXPROJ = ConcepttoClinic
SOURCEDIR = .
BUILDDIR = _build
APIDOCDIR_INTERFACE = _apidoc_interface
APIDOCDIR_PREDICTION = _apidoc_prediction

This comment has been minimized.

@WGierke

WGierke Aug 25, 2017

Contributor

Multiple apidoc directories are necessary since for each package a 'modules.rst' is created, which would be overwritten by the last execution of sphinx-apidoc when the output should only be saved in one directory.

@WGierke WGierke force-pushed the WGierke:8_sphinx_autodoc branch from f25b9ad to 5512b00 Aug 25, 2017

@lamby lamby merged commit df909ad into drivendataorg:master Aug 26, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
concept-to-clinic/cla @WGierke has signed the CLA.
Details
@lamby

This comment has been minimized.

Copy link
Contributor

lamby commented Aug 26, 2017

Thanks @WGierke !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment