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

Sphinx documentation. #2046

Merged
merged 3 commits into from
Apr 19, 2017
Merged

Conversation

oliver-sanders
Copy link
Member

@oliver-sanders oliver-sanders commented Feb 1, 2017

NOT READY FOR REVIEW.

Closes #1686

This change adds API reference pages auto-generated by the Sphinx (GPL) documentation builder.

  • Examples provided in doctest format are tested (via Sphinx) in t/docs/03-sphinx-doctests.t.
  • Documentation is built within a virtualenv (sphinx is installed via pip) to avoid installation requirements.
    • Wrapper script for building docs can be found at doc/bin/make-docs. Use make -C doc html to build.
    • Options for deployment:
      • Supply the pre-built docs.
      • Supply the wrapper script to build using a virtualenv. Implemented via makefile
      • Rely on user installation.
  • I've put together some quick developer notes at doc/sphinx/_build/sphinx.html (have to be built).
  • Thus far I have documented rose.config.*, rose.macro.MacroBase, rose.macro.MacroReporter.
  • Sphinx has a built-in search feature!

At present Sphinx is being used for its autodoc feature but is capable of building entire sites (e.g. www.docs.python.org).

Currently the default Sphinx theme is being used (same as the python2 docs), other themes are available, see in particular:

There are also two small behaviour changes brought in by this pull:

  • Rose macro will now exit upon encountering a macro without a docstring line (before there was traceback)
  • The kwarg keys in the rose.config.ConfigNodeDiff.(set_added_setting, set_modified_setting, set_removed_setting) methods can now be provided as a list as well as a tuple for consistency with rose.config.ConfigNode.

A bit of polish is still required - putting this up for comment @metomi/core.

  • Building of docs needs to be automated in our git workflow Ditched Travis build/deploy scripts
  • Build scripts require improvement Transitioned to makefile

@oliver-sanders
Copy link
Member Author

Updates:

  • Documentation themed with alabaster, a plain minimalist template (happy to change).
  • Build scripts migrated to a Makefile, run make -C doc html [doctest] to build.

Info:

  • Documentation is built to doc/sphinx/_build.
  • Some notes on Sphinx and docstrings can be found at doc/sphinx/_build/sphinx.html (have to be built).
  • Sphinx pages cannot be included in the single page documentation so I have written a place-holder for the Config API page.

Automated deployment:

  • It if possible to auto build/deploy via Travis-Ci but it requires giving Travis [force] push access via a user token (so that it can nuke the gh-pages branch).
  • For completeness the following is an outline of how to do this:

travis.yml

install:
    ...
    - pip install sphinx

script:
    - make -C doc doctest html && deploy-script "${PWD}"
    - rose test-battery ...

deploy-script

#!/bin/bash

ROSE_DIR=$1

# Only deploy on merge to master.
if [[ "${TRAVIS_BRANCH}" != 'master' || \
      "${TRAVIS_PULL_REQUEST}" != 'false' ]]; then
    # Documention should not be deployed.
    echo 'Skipped Deployment.'
    exit 0
fi

git clone 'https://github.com/davisp/ghp-import'

echo 'Deploying documentation ...'

# Place docs in "doc" directory for historic reasons.
echo -e '\tSymlinking documentation into site structure...'
GHP_DIR="$(mktemp -d)"
ln -s "${ROSE_DIR}/doc" "${GHP_DIR}/doc"
ln 'index.html' "${GHP_DIR}/index.html"

# Push new documentation.
echo -e '\tConstructing gh-pages branch...'
./ghp-import/ghp_import.py -ln "${GHP_DIR}"

echo -e '\tpushing gh-pages branch...'
git push -fq "https://${GH_TOKEN}@github.com/${TRAVIS_REPO_SLUG}.git" gh-pages

echo -e '\tDeployment successfull.'

setup:

  • Generate a github user token with public_repo authorisation.
  • Create an environment variable in Travis called GH_TOKEN, git it the value <user>:<token>.

@oliver-sanders
Copy link
Member Author

oliver-sanders commented Mar 22, 2017

Note the test battery now builds the docs.

Copy link
Member

@matthewrmshin matthewrmshin left a comment

Choose a reason for hiding this comment

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

2 minor issues:

  • venv/ directory not cleaned up.
  • t/syntax/01-doc.t failed in my environment.

Looks good otherwise.

@oliver-sanders oliver-sanders deleted the doc-sphinx branch March 22, 2017 13:00
@oliver-sanders oliver-sanders restored the doc-sphinx branch March 22, 2017 13:44
@matthewrmshin
Copy link
Member

Test not happy.

@oliver-sanders
Copy link
Member Author

Test now ecstatic.

@matthewrmshin matthewrmshin requested a review from arjclark March 28, 2017 11:14
@matthewrmshin
Copy link
Member

@arjclark please sanity check.


"""
node = self.load(source, node)
if not node:
node = self.load(source, node)
Copy link
Member

Choose a reason for hiding this comment

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

Just noticed that this change will break loading of site/user global configuration. After this change, user global will never get loaded because node will be defined. Please revert.

    * Fixes traceback for a rose macro without a docstring
Copy link
Contributor

@arjclark arjclark left a comment

Choose a reason for hiding this comment

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

Various minor comments. @matthewrmshin, as dicussed, can you double check the docsting entries for confignode are consistent with behaviour?

@@ -785,7 +785,8 @@ <h2 id="macro">Rose Macros</h2>
e.g. adding/removing options.</li>

<li>Upgraders - these are special transformer macros
for upgrading and downgrading configurations.</li>
for upgrading and downgrading configurations. (covered in the
<a href="#upgrade">next section</a>)</li>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you describe this as the "upgrade" section rather than "next"

</div>

<div class="panel-body">
<p>View the API docs <a href="rose-config-api.html">here</a>.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please close the <p> tag for consistency


# General information about the project.
project = u'rose-api-documentation'
copyright = u'2017, Met Office'
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do some strings in here have the unicode markup while others don't?

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 is the way Sphinx auto-generates them, I don't imagine it's necessary though.


""" # Blank line.

def __init(self, param1, param2, kwarg=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be __init__?


Args:
filename (str): The filename of the resource to request the path
to.
Copy link
Contributor

Choose a reason for hiding this comment

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

Alignment of this looks odd - maybe in line with the "T"

Copy link
Member Author

@oliver-sanders oliver-sanders Apr 5, 2017

Choose a reason for hiding this comment

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

This is the google docstring convention (overflow lines should have an extra 4 space indentation) see the guide I wrote in doc/sphinx/sphinx.rst and also http://sphinxcontrib-napoleon.readthedocs.io/en/latest/example_google.html.

@@ -459,13 +996,36 @@ def get_reversed(self):
return rev_diff

def delete_removed(self):
"""Deletes all 'removed' keys from this diff."""
"""Deletes all 'removed' keys from this ConfigNodeDiff..
Copy link
Contributor

Choose a reason for hiding this comment

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

Only the one "."

@@ -963,6 +970,8 @@ <h4 id="macro:code:api">API Reference</h4>

<pre class="prettyprint">
def report(self, config, meta_config=None):
""" Write some information about the configuration to a report file."""
# Note: report methods do not have a return value.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you combine the docstring and the immediately following # comment

@arjclark
Copy link
Contributor

Assuming @matthewrmshin has confirmed the ConfigNode docstring entries are consistent with behaviour then good to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants