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

set up documentation using mkdocs #215

Merged
merged 1 commit into from
Feb 7, 2025
Merged

Conversation

elfkuzco
Copy link
Contributor

@elfkuzco elfkuzco commented Jan 30, 2025

Rationale

Make API documentation online via readthedocs.com. This resolves #186

Changes

  • Add documentation for all classes and methods.
  • Harmonize docstrings towards google style.
  • Hide documentation of classes and methods from showing in reference home page as they are defined in their respective modules. This is a consequence of the force_inspection flag being set to true.
  • use zim.Entry instead of zim::Entry to refer to C++ objects because of formatting issues with docstring style.

Limitations

Given the docstrings are defined in libzim.pyx files and not in the .pyi files, documentation was built by setting the force_inspection flag of griffe to true. This forces griffe (internal tool used by mkdocs) to inspect the compiled module. We merely used the .pyi files to generate the names of the reference pages. As a consequence, there are a few limitations to be aware of:

@elfkuzco
Copy link
Contributor Author

Recently, one of the issues in the limitations section has been resolved and I have to edit some of the docs. Will submit the updates subsequently to this PR. So, it should be held off as a draft.

@elfkuzco elfkuzco marked this pull request as draft January 31, 2025 11:31
@rgaudin
Copy link
Member

rgaudin commented Feb 4, 2025

OK let me know once OK ; I've reviewed most of it

@elfkuzco
Copy link
Contributor Author

elfkuzco commented Feb 5, 2025

OK let me know once OK ; I've reviewed most of it

OK. It's ready except for the issue mkdocstrings/griffe#355

I think it might have do to with cython behaviour than mkdocstrings. I followed the hints that the author left in the conversation but I can't seem to get it working with merge_init_into_class. Could you please check the conversation and see if it's something that we can do on our side?

Copy link
Member

@rgaudin rgaudin left a comment

Choose a reason for hiding this comment

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

Thank you very much for this tedious work.

I don't see much dosctrings anymore though. We even lost the module level docstrings ; that's a problem as those are the most important and contain examples.

Could you check? I re-ran build-ext then used mkdocs serve.

Screenshot 2025-02-06 at 11 30 24

@elfkuzco
Copy link
Contributor Author

elfkuzco commented Feb 6, 2025

Regarding the docstrings not showing properly, I forgot to update the version number of the dependencies after I submitted a PR for mkdocstrings[python]. I forgot I had edited the version in the venv while testing. Things ought to work properly now.

Aside from the issue with cython, everything works fine and it's very much complete. If it's ready for merging, I can squash the commits then.

@elfkuzco elfkuzco requested a review from rgaudin February 6, 2025 13:04
@elfkuzco elfkuzco marked this pull request as ready for review February 6, 2025 13:09
Copy link
Member

@rgaudin rgaudin left a comment

Choose a reason for hiding this comment

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

Thank you ; that looks quite nice!

I have one last suggestion now.

With your current docstrings, those attributes for which we cant get return types directly appear like this:

Screenshot 2025-02-06 at 13 17 51

If we dont put the type in the leading part of the dosctring but in a Returns section, it looks like this:

Screenshot 2025-02-06 at 13 16 07

There sure is duplication of that return value description but I think it looks more practical. What do you think?

Here's the diff for that example.

diff --git i/libzim/libzim.pyx w/libzim/libzim.pyx
index b5960ea..71ef204 100644
--- i/libzim/libzim.pyx
+++ w/libzim/libzim.pyx
@@ -1149,10 +1149,13 @@ cdef class Archive:
 
     @property
     def entry_count(self) -> pyint:
-        """int: Number of user entries in Archive.
+        """Number of user entries in Archive.
 
         If Archive doesn't support “user entries”
         then this returns `all_entry_count`.
+
+        Returns:
+            (int): Number of user entries in Archive.
         """
         return self.c_archive.getEntryCount()

Whether you switch to it or not, you can rebase or squash then and we'll merge.

@elfkuzco
Copy link
Contributor Author

elfkuzco commented Feb 6, 2025

I have duplicated the docstring in the Returns section. Seems like the authors of mkdocstrings forgot to inspect the annotation of the GetSet descriptor which is what @property turns to for compiled modules. Will make a PR for them later and come back to this but it seems okay now.

@elfkuzco elfkuzco requested a review from rgaudin February 6, 2025 14:34
Copy link
Member

@rgaudin rgaudin left a comment

Choose a reason for hiding this comment

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

👍

@rgaudin
Copy link
Member

rgaudin commented Feb 7, 2025

It's failing only on aarch64 for Python3.13. Opened #216

@rgaudin rgaudin merged commit c6634cc into openzim:main Feb 7, 2025
1 check passed
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.

Make an API online documentation
2 participants