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

Create documentation with sphinx #248

Closed
wants to merge 0 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@chrta
Contributor

chrta commented Mar 23, 2018

This is a first shot for #235

Only some initial work is done for now.

  • The project was created with sphinx-quickstart
  • readthedocs theme is used
  • "make html" works
  • the old functionality to take screenshots and execute tests should be preserved
The following python packages have to be installed, e.g. with pip:
pip install sphinx sphinx_rtd_theme

This comment has been minimized.

@chrta

chrta Mar 23, 2018

Contributor

Or should this be executed in a virtualenv and requirements.txt should be provided?

This comment has been minimized.

@latk

latk Mar 23, 2018

Member

Currently all development prerequisites are documented in the CONTRIBUTING document. You can mention the sphinx dependencies there. If you list the deps in a requirements.txt that would probably be even better (in which case CONTRIBUTING and doc/README and possibly also .travis.yml and appveyor.yml can be simplified).

This comment has been minimized.

@chrta

chrta Mar 24, 2018

Contributor

I added a requirements.txt without pinning the versions.

$(SCREENSHOT_FROM_HTML) $< $@
examples/example-%.html:
cd examples; ./example_html.sh
outputs:
../scripts/gcovr -h > examples/gcovr.out
gcovr -h > examples/gcovr.out

This comment has been minimized.

@chrta

chrta Mar 23, 2018

Contributor

How does gcovr end up in ../scripts? Maybe this is missing in some documentation or i did not find it.

This comment has been minimized.

@latk

latk Mar 23, 2018

Member

Thank you for spotting this problem. Originally gcovr was a single file in the scripts directory. Fairly recently, it was refactored into a proper module that provides a gcovr executable upon installation. So your change is correct.

@codecov

This comment has been minimized.

codecov bot commented Mar 23, 2018

Codecov Report

Merging #248 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #248   +/-   ##
=======================================
  Coverage   88.76%   88.76%           
=======================================
  Files          13       13           
  Lines        1477     1477           
  Branches      267      267           
=======================================
  Hits         1311     1311           
  Misses        108      108           
  Partials       58       58

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 869bbc8...517c04e. Read the comment docs.

@latk

This comment has been minimized.

Member

latk commented Mar 23, 2018

Fantastic! Thank you for working on this. I'll look into this in the evening.

@chrta

This comment has been minimized.

Contributor

chrta commented Mar 24, 2018

How about the structure used in http://sphinx-rtd-theme.readthedocs.io/en/latest/index.html and https://raw.githubusercontent.com/rtfd/sphinx_rtd_theme/master/docs/index.rst?
The readme at https://github.com/rtfd/sphinx_rtd_theme is very short and points to all relevant documents. The detailed documentation is only available at readthedocs.io

If i include the README.rst as it is now, the paths to the images do not work and the toc structure is not very nice.

@latk

This comment has been minimized.

Member

latk commented Mar 24, 2018

Hmm, that's a difficult problem. I would strongly prefer to keep the README useful without having to follow external links.

Perhaps an alternative would be to move the documentation directory to the root directory? If the doc directory gets in the way, we can get rid of it. Then, the README.rst could be used as the master document instead of a doc/index.rst? What do you think?

@chrta chrta force-pushed the chrta:doc_to_sphinx branch from 70afabd to 46754a5 Mar 25, 2018

@chrta

This comment has been minimized.

Contributor

chrta commented Mar 25, 2018

I would keep the documentation in the docs folder. Otherwise the sphinx files, makefile etc would be in the same folder as non-doc stuff.

Maybe we can include snippets from the main README.rst in the doc/source/index.rst file, like in the guide.rst file:

.. include:: ../../README.rst
   :start-after: .. begin abstract
   :end-before: .. end abstract

This way we would reduce copy-and-paste an little bit and can setup a documentation structure freely for sphinx.

See the last commit e0efb49.

@chrta chrta changed the title from WIP: Create documentation with sphinx to Create documentation with sphinx Mar 28, 2018

@latk

I'm now reviewing this PR so that I can merge it – sorry for the delay, I've been very busy.
I think the new documentation looks absolutely fantastic, and I'm looking forward to replacing the homepage :)

Below, I've gathered a few review comments. The first bunch is about the Sphinx configuration, the rest about the structure and organization of the docs.
I'd be super glad if you could tackle these suggestions. In any case, I'll try to merge within a week.

project = u'gcovr'
copyright = u'2018, William Hart, John Siirola and and Lukas Atkinson.'
author = u'William Hart, John Siirola and and Lukas Atkinson.'

This comment has been minimized.

@latk

latk Apr 2, 2018

Member

Duplicate "and and". Also, I'd rather credit the copyright and authorship to “the gcovr authors”, not to a few individuals.

# The short X.Y version
version = u''
# The full version, including alpha/beta/rc tags
release = u''

This comment has been minimized.

@latk

latk Apr 2, 2018

Member

Can the version/release info somehow be discovered automatically? If not, I'm fine with leaving these fields empty for now.

This comment has been minimized.

@chrta

chrta Apr 3, 2018

Contributor

We could get this information from the last git tag on this branch using git describe:

#to get the git revision (see http://dreamiurg.net/2011/10/03/using-git-to-get-version-information/)
from subprocess import Popen, PIPE

def get_version():
    #"""
    #Returns project version as string from 'git describe' command.
    #"""
    pipe = Popen('git describe --tags --always', stdout=PIPE, shell=True)
    version = pipe.stdout.read()

    if version:
        return version.decode('utf-8')
    else:
        return 'X.Y'

version = get_version().rstrip()
release = version

Is this ok?

This comment has been minimized.

@mayeut

mayeut Apr 3, 2018

Contributor

Why not use the same kind of thing as in setup.py:

gcovr/setup.py

Line 35 in 869bbc8

version = run_path('./gcovr/version.py')['__version__']

This comment has been minimized.

@chrta

chrta Apr 3, 2018

Contributor

This should work. But using ./gcovr/version.py would always report 3.4 for every version after 3.4. git describe --tags --always will report something like 3.4-65-g869bbc8 if you are not exactly on a tag. So you see, that it is an intermediate version.

This comment has been minimized.

@latk

latk Apr 3, 2018

Member

Thank you for everyone's suggestions!

Right now, the guide is only published manually for a release, so that the published guide always refers to the most recent released version.
In the long run, I'd like to automate this so that we can publish the docs for both the last release and the current development version (via readthedocs?). In either case, I'm not sure that tying a documentation build to a git commit is valuable.

If there's no simple way to include a version, I'd rather keep this blank for now and revisit this if and when giving a clear version would be necessary. E.g. I think readthedocs wouldn't use the project version from the sphinx config at all.

# author, documentclass [howto, manual, or own class]).
latex_documents = [
(master_doc, 'gcovr.tex', u'gcovr Documentation',
u'William Hart, John Siirola and and Lukas Atkinson.', 'manual'),

This comment has been minimized.

@latk

latk Apr 2, 2018

Member

duplicate title and author info – consider using the variables at the top. Same for "man_pages", "texinfo_documents" entries below.

@@ -205,7 +203,7 @@ This generates a HTML summary of the lines executed. In this
example, the file ``example1.html`` is generated, which has the
following output:
.. image:: ./screenshot-html.png
.. image:: ./_static/screenshot-html.png

This comment has been minimized.

@latk

latk Apr 2, 2018

Member

It seems Sphinx places two copies of the screenshots into the build folder: one direct copy in the _static folder, and another copy in an _images folder. Given that Sphinx takes care of the screenshot URL anyway, can we leave the screenshots in the top folder?

This comment has been minimized.

@chrta

chrta Apr 3, 2018

Contributor

So you would like to put the images, that are generated by the makefile (and checked in), in the folder doc/source/ or doc/ instead of doc/source/_static?

This comment has been minimized.

@latk

latk Apr 4, 2018

Member

Yes. I'd probably leave them in doc/. Which directory do you think would be best?

This comment has been minimized.

@chrta

chrta Apr 5, 2018

Contributor

I would prefer a separate folder, only for the images. I like to have generated files that may be generated by the build system (like these images) in an extra folder, may be doc/images or something like this.

This comment has been minimized.

@latk

latk Apr 6, 2018

Member

That's much better than my suggestion!
Ok, I think fixing the image location is the last change I'd like to see before I merge the PR.

.. include:: ../../README.rst
:start-after: .. begin abstract
:end-before: .. end abstract

This comment has been minimized.

@latk

latk Apr 2, 2018

Member

When I change the homepage to the Sphinx-generated version, I'd still like to keep the links from the current homepage here. Can we add a paragraph with the following links here so that they are still visible “above the fold”?

Though it might be better to include the changelog within the documentation – I think we'd only have to fix the formatting to use Restructured Text.

This comment has been minimized.

@chrta

chrta Apr 3, 2018

Contributor

I included the changelog as it is now in the toctree. Later on it can be reformatted into proper rst.

This comment has been minimized.

@latk

latk Apr 4, 2018

Member

I translated the changelog in chrta#1, you can merge my PR into your branch to include those changes as part of this PR.

quickstart
contributing
license
guide

This comment has been minimized.

@latk

latk Apr 2, 2018

Member

For the initial switch to Sphinx-based docs, I'd initially avoid separate chapters for installation & quickstart since there are similar sections in the guide. We can later refactor that.

Since the guide is the most important part, I'd put it first:

  • guide
  • contributing
  • (changelog?)
  • license

What do you think of this structure?

@chrta chrta force-pushed the chrta:doc_to_sphinx branch from 100d647 to cda47e9 Apr 3, 2018

@latk latk referenced this pull request Apr 4, 2018

Merged

Translate changelog to ReST #1

@chrta

This comment has been minimized.

Contributor

chrta commented Apr 6, 2018

Everything should be done now.

@latk

latk approved these changes Apr 6, 2018

@latk

This comment has been minimized.

Member

latk commented Apr 6, 2018

Ok, I'll do the merge over the weekend. Thank you very much for all your help with this issue :)

latk added a commit that referenced this pull request Apr 6, 2018

@latk latk closed this Apr 6, 2018

@latk latk force-pushed the chrta:doc_to_sphinx branch from 517c04e to 3d44d59 Apr 6, 2018

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