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

Add docs #4

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Add docs #4

wants to merge 9 commits into from

Conversation

LiamPattinson
Copy link
Contributor

Updates docstrings and adds HTML documentation with Sphinx.

  • Docstrings rewritten to use NumPy doc style
  • Updates type-hints throughout, as these are used by the Sphinx plug-in sphinx_autodoc_typehints.
  • Adds Sphinx files in the docs/ directory (mostly boilerplate, though conf.py and index.rst may need to be edited from time to time)

The majority of this was completed by @ZedThree -- I've simply merged it with the latest main and checked for bugs.

Docs can be built locally by the following steps:

pip install .[docs]  # Get the documentation dependencies
cd docs
make html

You can then check them by navigating to file:///path/to/simplesimdb/docs/_build/html/index.html in your browser.

@mwiesenberger To set up auto-publishing on readthedocs, you'll have to follow the tutorial from here. I'd recommend setting it to build pull requests, but otherwise the default set up should be fine.

Copy link
Owner

@mwiesenberger mwiesenberger left a comment

Choose a reason for hiding this comment

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

Hi, I finally found some time to look into this and play around with it. I have some requests that I tried unsuccessfully to figure out myself so I would appreciate if you with more experience can get it to work

docs/conf.py Outdated
# https://www.sphinx-doc.org/en/master/usage/configuration.html#project-information

project = "simplesimdb"
copyright = "2024, Matthias Wiesenberger"
Copy link
Owner

Choose a reason for hiding this comment

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

This should copy the corresponding line in ../LICENSE
copyright = "(c) 2021 Technical University of Denmark"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've corrected this in 481bfc2. The "(c)" wasn't needed, as one is added automatically in the docs.

Copy link
Owner

Choose a reason for hiding this comment

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

Perfect

@@ -0,0 +1,32 @@
{{ fullname | escape | underline}}
Copy link
Owner

Choose a reason for hiding this comment

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

I am not sure what the two files in _templates bring to the table or where they come from? When I run sphinx-quickstart they are not generated, so do they somehow overwrite the default behaviour?

Copy link
Contributor

Choose a reason for hiding this comment

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

Without these custom templates, Sphinx doesn't actually have a way of automatically and recursively generating all the docs. These versions are just modified from the default templates to do that recursive autogeneration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They're used by the sphinx.ext.autosummary plugin to automate the generation of module and class documentation. The autodoc plugin is used to automate documentation generation on a per-module/per-class/per-function/etc basis, and autosummary is used to generalise autodoc over all modules/classes/functions in a project.

I think these files were simply copied across from another project. Once you've written documentation for one Python project, it makes a lot of sense to just copy all the boilerplate over to the next, but sometimes you end up bringing over things that aren't strictly necessary.

With the changes I've made I don't think the module template will be needed anymore, so I can delete that if you'd like. Alternatively, it might be worth keeping around just in case the project expands at a later date.

The class template is needed in order for the documentation for Manager and Repeater to be created successfully. If you exclude it, the class itself gets documented but none of its methods do.

Copy link
Owner

@mwiesenberger mwiesenberger Jan 27, 2025

Choose a reason for hiding this comment

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

Without these custom templates, Sphinx doesn't actually have a way of automatically and recursively generating all the docs

Hmm why not use

.. automodule:: simplesimdb
    :members:
    :undoc-members:
    :show-inheritance:

For me that creates all classes and Members even without the files in _templates

Copy link
Owner

Choose a reason for hiding this comment

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

I compared custom-class-template.rst to sphinx's default /autosummary/class.rst and the only difference is the addition of

:members:
:undoc-members:
:show-inheritance:

The module custom-module-template.rst compared to sphinx's default /autosummary/module.rsttemplate includes

:toctree

at a couple of places. I think I am ok with just using Sphinx default behaviour for now, if you remove the two _template files for me and include the above automodule for me I can accept the pull request and then play around a bit with the documentation from there and put it onto readthedocs

docs/index.rst Outdated
parameter scans with a few lines of Python code **without manual
interference**. In this way for example publication grade plots can
be (re-)produced from scratch by just executing Python scripts. (See
for example the `impurities project <https://github.com/mwiesenberger/impurities>`_)
Copy link
Owner

@mwiesenberger mwiesenberger Jan 23, 2025

Choose a reason for hiding this comment

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

I see that you copied a passage of the README.md file here, but I would rather that the README is copied verbatim after the toctree. It should be possible since myst_nb is included as an extension but I couldn't get it to work myself

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I honestly wasn't aware that this was even a possibility, but I've got it working in f169a24. I'll have to remember this trick for my future projects!

Copy link
Owner

Choose a reason for hiding this comment

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

Glad I could point out something useful. Looks good now

# The full version, including alpha/beta/rc tags
release = get_version(project)
# Major.minor version
version = ".".join(release.split(".")[:2])
Copy link
Owner

Choose a reason for hiding this comment

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

Can "version" be used in the html_title since the full "1.1.2.dev5+gd676620" looks a bit ugly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've made the change in 1887a1b. A downside of this is that there won't be any way to tell if a particular version of the docs have been built from the latest release or a mid-development commit, but it's unlikely that users will stumble onto these development builds anyway.

Copy link
Owner

Choose a reason for hiding this comment

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

For me the important thing is also to see how it can be done and you showed an easy and elegant solution that I can play around with in the future, should it become an issue

docs/api.rst Outdated
@@ -0,0 +1,10 @@
===============
Copy link
Owner

Choose a reason for hiding this comment

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

I am not entirely sure about the purpose of this file, in my mind the simple thing would be to just include the simplesimdb module in index.rst somehow, or even better have the Reference to the two classes Manager and Repeater go directly on the main page. The "API Reference" just creates another level of indirection to click through...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It took some experimentation, but I was able to achieve this in 9ceab46. It looks like the general layout of the docs had been copied across from another Python project that made use of many more modules, and in that case the indirection may have made more sense to avoid cluttering the main page.

Copy link
Owner

Choose a reason for hiding this comment

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

Perfect, yes this is what I wanted, I think from here I can figure out how to do more elaborate things in sphinx should it be necessary

- method: pip
path: .
extra_requirements:
- docs
Copy link
Owner

Choose a reason for hiding this comment

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

Do I understand this correctly that it will use the requirements in the [docs] group of pyproject.toml?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's right, it's just a way to avoid repeating our list of documentation dependencies.

Copy link
Owner

Choose a reason for hiding this comment

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

Nice

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