-
Notifications
You must be signed in to change notification settings - Fork 16
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
Auto-generate API index page #234
Conversation
e7052c4
to
1dcdb00
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #234 +/- ##
=======================================
Coverage 99.74% 99.74%
=======================================
Files 13 13
Lines 771 771
=======================================
Hits 769 769
Misses 2 2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great! 🚀
Make is out of my depth but I made a couple suggestions. But everything works well locally for me ✨
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice work @lochhh! I guess it makes sense to first merge the actions PR, modify this one accordingly and then follow up with merging this one?
os.chdir(script_dir) | ||
|
||
|
||
def make_api_index(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way we can define a custom ordering of modules, if we wish to ever override the default alphabetical ordering?
On one hand having a custom order would allow us to highlight the "important" modules near the top. But on the other hand, having that custom step will introduce manual work when creating/deleting a module, and the whole point of this PR was to eliminate manual work. So I think I'm fine with this staying alphabetical.
What do you think?
6d3df37
to
1dcdb00
Compare
I've merged the proposed upstream change in |
1dcdb00
to
10181bd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @lochhh!
I just had two final comments.
1c0f101
to
84f0f0a
Compare
|
Description
What is this PR
Why is this PR needed?
Currently, one needs to manually update entries in the
docs/source/api_index.rst
page, so that new modules or renamed (existing) modules are included in the API reference. This PR automates the generation of thisapi_index.rst
page using themake_api_index.py
script, such that all modules are automatically included, except for what's specified in the exclusion list in this script.What does this PR do?
make_api_index.py
script and templateapi_index_head.rst
for auto-generating theapi_index.rst
filedocs/
directory (this is used in the build docs workflow)make html
instead ofpython make_api_index.py && sphinx-build source docs/build
make clean html
instead ofrm -rf [all auto-generated files] && python make_api_index.py && sphinx-build source build
make linkcheck
instead ofsphinx-build source build -b linkcheck
use-make
input option of the NIUbuild_sphinx_docs
anddeploy_sphinx_docs
actions in the build docs workflow to enable docs to be generated using themake
utilityapi_index.rst
and its tracking, as this is now auto-generatedHow has this PR been tested?
Docs build successfully both locally and remotely. See also live docs on forked repo.
Does this PR require an update to the documentation?
I've updated the "API reference" section in
CONTRIBUTING.md
accordingly. For the "building docs locally" instructions, I've gone with the longer but more genericpython docs/make_api_index.py && sphinx-build docs/source docs/build
rather than the shortermake html
as the command would be different for windows vs Unix users.make
is also not necessarily available on all systems and may require additional installation. Let me know your thoughts on this.TODO:
Checklist: