-
Notifications
You must be signed in to change notification settings - Fork 25
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 API reference generated by Sphinx #212
base: main
Are you sure you want to change the base?
Conversation
This is `sphinx-quickstart` as modified to automatically produce documentation for the module, taking a few hints from fonttools/ufoLib2 to inform structure, build workflow, and styling.
Having an explicit __all__ is necessary to avoid re-exporting objects that we ourselves have imported (e.g. from the standard library). This also helps avoid objects that this project did not author from appearing in the automatically generated Sphinx documentation. This list was generated by: ```python import uharfbuzz print(dir(uharfbuzz)) ``` ...before being manually pruned.
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 good, thanks!. CI integration would be welcome. We probably should also publish it to readthedocs.io.
"ot_math_get_glyph_variants", | ||
"ot_math_get_min_connector_overlap", | ||
"ot_math_has_data", | ||
"ot_math_is_glyph_extended_shape", |
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.
All the ot_*
functions above are deprecated, do we still need to include them here?
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.
I've just tested locally, and if we exclude the functions here they will be excluded from documentation, but would also be excluded from any existing star-imports (i.e. from uharfbuzz import *
), and so it would be a breaking change.
Maybe this is okay if we release explicitly as breaking and there are alternatives though? (or even as v1.0.0, if the module enjoys any production use?)
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.
Alternatively there may be a way to make them show as deprecated in Sphinx
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.
(in some ways, even adding __all__
at all is already a breaking change, but the only variables being excluded at the moment are re-exports from uharfbuzz's dependencies and the standard library, which I think we can consider a bug fix)
docs/source/conf.py
Outdated
|
||
project = "uharfbuzz" | ||
copyright = "2024, Adrien Tétar" | ||
author = "Adrien Tétar" |
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.
May be we can use something more generic, like “HarfBuzz team” or something? Adrien hasn’t been involved with this project for a while now.
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.
I've now changed the author
to HarfBuzz Contributors
to adopt this 👍
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.
(as I am a lowly first-time contributor I've left the copyright notice as-is, but have checked "Allow edits by maintainers" on this PR in case any maintainer wishes to set it)
Thanks for the speedy review Khaled 💪 I'll see what it would take to run the |
This is following the guidance at: https://docs.readthedocs.io/en/stable/guides/reproducible-builds.html ...in preparation for adding Read the Docs build support, where having an explicit requirements.txt file to point to will also be helpful.
This is based on the Sphinx reference config file here: https://docs.readthedocs.io/en/stable/config-file/v2.html#python-install ...with some trimming, customisation for the project, and additional adjustments recommended for reproducible builds.
I've added a template Would someone with maintainer access be willing to follow the Read the Docs procedure required to create a sub-domain and have it insert actions into this repository to populate the docs? Side note: if the pull request option is available for community plans then we may be able to avoid the duplicate runner effort of including a CI job for docs on GitHub too 🚀 |
Hello - I've had a stab at #206!
This is largely a modification of
sphinx-quickstart
to automatically generate an API reference for the module, taking a few hints from fonttools/ufoLib2 to inform structure, build workflow, and styling.It can be tested as follows:
...and seems to be fairly happy with Cython, aside from a known upstream issue with displaying the types of properties (sphinx-doc/sphinx#7448).
In general, the generated documentation will remain sparse until more docstrings are added, however, it will hopefully still deliver an improvement over reading the sources, especially with the built-in search functionality that Sphinx provides :)
Known caveats:
setup.py
; are these the most accurate to use?Note: this is a personal contribution independent of my employer, and so I've submitted from a fork under my personal profile and email to make this distinction ✔️