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

Improve documentation, add documentation testing, and make all examples into scripts. #101

Merged
merged 9 commits into from
Apr 22, 2021

Conversation

erikmannerfelt
Copy link
Contributor

@erikmannerfelt erikmannerfelt commented Apr 21, 2021

This was a hard one, including environment variables and all kinds of weird errors!

Fixed warnings and incorrect docstrings

There were quite many warnings before when building the documentation. I've made sure that all of them have been fixed.

Documentation build tests

I've added tests for all example scripts (I'll get more to that in a second) and for building the documentation. Now, theoretically, ReadTheDocs should always build if the build check on GitHub passes. Makes for easier maintainability in the long term, as a PR that passes the build check should be compatible with ReadTheDocs (right now, builds might pass but then the documentation fails to build!).

More future-frendly examples structure.

Finally, since we already have had trouble with outdated examples, all example code is now in docs/source/code/*.py. This has many advantages:

  1. Linters and formatters can locally help with the scripts to make sure that they're correct.
  2. It's easier to debug, as the code can be run without rebuilding the entire documentation.
  3. Inline code in rst is awkward to write
  4. It allows testing the code externally (if an example is outdated, it won't pass the build check!)

See the result

If you're interested, you can see how the documentation will look here:
https://xdem-erikm.readthedocs.io/en/improve_docs/index.html

@erikmannerfelt erikmannerfelt added documentation Improvements or additions to documentation enhancement Feature improvement or request labels Apr 21, 2021
@erikmannerfelt erikmannerfelt self-assigned this Apr 21, 2021
@erikmannerfelt
Copy link
Contributor Author

This partly tackles #53 as invalid docstrings will show up in the build check (as warnings if they are minor, and failed builds if they are major).

#75 and #92 were only needed because we didn't have a good validation technique for the documentation examples. This should solve it.

Copy link
Member

@rhugonnet rhugonnet left a comment

Choose a reason for hiding this comment

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

Nothing to say except that this is going to be super useful!

I'm wondering if this kind of "good practice" won't eventually (in 5-year time?) be included in a more user-friendly way to GitHub. I hope so.
The advantage is that we really get to know how this all works (well.. mostly you Erik!)

@@ -9,35 +9,9 @@ DEM subtraction and volume change

Example data in this chapter are loaded as follows:

.. code-block:: python
.. literalinclude:: code/comparison.py
Copy link
Member

Choose a reason for hiding this comment

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

so literalinclude is kind of the key here :D

"""
Make sure readthedocs finds the module.

Maybe this is not needed?
Copy link
Member

Choose a reason for hiding this comment

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

time will tell !

import warnings


class TestDocs:
Copy link
Member

Choose a reason for hiding this comment

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

very nice

@erikmannerfelt
Copy link
Contributor Author

Thank you for the feedback, @rhugonnet!

I wanted to see if other packages used similar testing, so I checked rasterio.

  1. They recommend installing rio=0.24 and gdal=1.11 in their installation page
  2. I haven't found any incorrect syntax, but many statements are only really pseudocode. See here; what are the kwargs!? (turns out they're width, height, cound, transform, dtype and crs...)
  3. There are some typos here and there; dtype-uint8 in the output instead of dtype=uint8.

@erikmannerfelt erikmannerfelt merged commit 64f08b0 into GlacioHack:main Apr 22, 2021
@adehecq
Copy link
Member

adehecq commented May 12, 2021

Thank you for the feedback, @rhugonnet!

I wanted to see if other packages used similar testing, so I checked rasterio.

1. They recommend installing rio=0.24 and gdal=1.11 in their [installation page](https://rasterio.readthedocs.io/en/latest/installation.html#windows)

2. I haven't found any incorrect syntax, but many statements are only really pseudocode. [See here](https://rasterio.readthedocs.io/en/latest/topics/switch.html#format-drivers); what are the kwargs!? (turns out they're width, height, cound, transform, dtype and crs...)

3. There are some typos [here and there](https://rasterio.readthedocs.io/en/latest/topics/switch.html#valid-data-masks); `dtype-uint8` in the output instead of `dtype=uint8`.

Does this mean we're now doing better than rasterio?! :-)

@erikmannerfelt erikmannerfelt deleted the improve_docs branch July 7, 2021 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement Feature improvement or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants