Skip to content

set up documentation using mkdocs#215

Merged
rgaudin merged 1 commit into
openzim:mainfrom
elfkuzco:online-api-doc
Feb 7, 2025
Merged

set up documentation using mkdocs#215
rgaudin merged 1 commit into
openzim:mainfrom
elfkuzco:online-api-doc

Conversation

@elfkuzco

@elfkuzco elfkuzco commented Jan 30, 2025

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

rgaudin commented Feb 4, 2025

Copy link
Copy Markdown
Member

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

@elfkuzco

elfkuzco commented Feb 5, 2025

Copy link
Copy Markdown
Contributor Author

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?

@rgaudin rgaudin left a comment

Copy link
Copy Markdown
Member

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

Comment thread mkdocs.yml Outdated
Comment thread libzim/libzim.pyx Outdated
Comment thread libzim/libzim.pyx Outdated
Comment thread libzim/libzim.pyx Outdated
Comment thread libzim/libzim.pyx Outdated
Comment thread libzim/libzim.pyx Outdated
Comment thread libzim/libzim.pyx Outdated
Comment thread libzim/libzim.pyx Outdated
Comment thread libzim/libzim.pyx
Comment thread libzim/libzim.pyx Outdated
@elfkuzco

elfkuzco commented Feb 6, 2025

Copy link
Copy Markdown
Contributor Author

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

@rgaudin rgaudin left a comment

Copy link
Copy Markdown
Member

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.

Comment thread CHANGELOG.md Outdated
@elfkuzco

elfkuzco commented Feb 6, 2025

Copy link
Copy Markdown
Contributor Author

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

@rgaudin rgaudin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

@rgaudin

rgaudin commented Feb 7, 2025

Copy link
Copy Markdown
Member

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

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