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 acquire-zarr docs #93

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

Conversation

melissawm
Copy link
Collaborator

@melissawm melissawm commented Dec 3, 2024

Will close #92

Still working on autogeneration of API documentation.

PREVIEW: https://melissawm.github.io/acquire-docs/dev/stream_to_zarr/

@melissawm
Copy link
Collaborator Author

I pushed changes to include the Python API to the Zarr library, but something is still wrong - the template I created to filter the attributed from the module is not working here. Might have somehting to do with how these objects are documented in the cpp file. I will investigate. Preview is updated: https://melissawm.github.io/acquire-docs/dev/stream_to_zarr/zarr_api/

@melissawm
Copy link
Collaborator Author

Here's a preview of a proof of concept for using Doxygen/mkdoxy to add the C API docs to the site: https://melissawm.github.io/acquire-docs/stable/stream_to_zarr/

This is not ideal as there as several points I would like to improve:

  1. The Python API pages (generated directly by mkdocstrings) show up as one big page with the contents, and the links for different objects appear in the in-page toc (right-side navbar). The C API pages (generated by Doxygen/mkdoxy) show up as one page per object, meaning those docs appear in the left-hand sidebar. This can be confusing.
  2. I haven't figured out how to explicitly order the C API pages to make them more organized. This should be possible with awesome-pages but - since the folder containing the C API pages is only generated on build and doesn't exist before mkdoxy runs, I haven't figured out a way to do it. Will continue to explore.
  3. There will probably be design feedback for these items, but I'm not sure there will be a way to make a full correspondence between the doxygen-generated docs and the mkdocs-generated docs for each object. We can discuss more on monday.

@melissawm melissawm force-pushed the add-zarr branch 2 times, most recently from 8a84ee2 to fd323c7 Compare January 23, 2025 14:38
* Add Python and C API docs for acquire-zarr
* Add doxygen-generated files to gitignore
* Add Doxyfile for more configuration options
* Remove unwanted items from docs
@melissawm melissawm marked this pull request as ready for review January 23, 2025 19:23
@melissawm
Copy link
Collaborator Author

Hi all! I made some progress on the items we discussed last time.

  1. I've managed to tweak the Doxygen config values to (hopefully) get closer to what we want. Please feel free to point out other improvements I can work on. For the "import" lines at the top of the two files we are showing, apparently it's a bug in doxygen - see Doxygen 1.13.0 ignores visibility="no" for various layout elements doxygen/doxygen#11301. I did try with 1.13.2 but I still see the same problem.
  2. Attributes are not shown individually anymore in the Python API documentation for Zarr streaming, they are shown in a table similar to what happens for the Acquire Python reference docs. However, there is a problem with how they are generated.

After some testing, I realized that after building and installing acquire_zarr on python results in the docstrings for the various objects defined for the library to come from https://github.com/acquire-project/acquire-zarr/blob/main/python/acquire-zarr-py.cpp and NOT https://github.com/acquire-project/acquire-zarr/blob/main/python/acquire_zarr.pyi as I previously assumed. I'm not sure if this is a result of the build process or if acquire_zarr.pyi can be regenerated from the cpp file. In addition, the attributes for each class are called "Members" and as such create some confusion in the docstring formatting. Since I don't know exactly how to edit the cpp file directly to change this, we get some weird docstrings here: https://melissawm.github.io/acquire-docs/dev/stream_to_zarr/zarr_api/#acquire_zarr.CompressionCodec

If you have any tips on how to solve this issue that would be great. Thanks!

@nclack nclack requested a review from aliddell January 28, 2025 21:22
Copy link
Member

@aliddell aliddell left a comment

Choose a reason for hiding this comment

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

Looks OK to me, just had one question.

Comment on lines 95 to 97
{# {% with docstring_sections = module.docstring.parsed %}
{% include "docstring"|get_template with context %}
{% endwith %}
{% endwith %} #}
Copy link
Member

Choose a reason for hiding this comment

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

What does this do here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh - I made these into comments because I was trying to remove the module docstring from rendering, since we already have an intro on the page and that seemed more appropriate than the docstring. I can delete entirely since it seems this is the way to go.

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.

Add Documentation for Zarr Streaming Library
2 participants