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

Make an API online documentation #186

Open
kelson42 opened this issue Jan 15, 2024 · 14 comments · May be fixed by #215
Open

Make an API online documentation #186

kelson42 opened this issue Jan 15, 2024 · 14 comments · May be fixed by #215
Assignees
Labels
Milestone

Comments

@kelson42
Copy link
Contributor

via readthedocs.com

@kelson42 kelson42 added the task label Jan 15, 2024
@kelson42 kelson42 added this to the 3.6.0 milestone Jan 15, 2024
@kelson42 kelson42 pinned this issue Sep 3, 2024
@rgaudin rgaudin modified the milestones: 3.6.0, 3.7.0 Oct 15, 2024
@elfkuzco
Copy link

@kelson42 , I would like to work on this. I don't have much experience with building the docs with readthedocs.com but I am aware you could either use Sphinx or MkDocs as the documentation generators. Do you have any generator in mind? I reckon I could get it done in a week or less if it is assigned to me.

@benoit74
Copy link

We need something similar to https://github.com/openzim/python-scraperlib/, which is based on mkdocs.

For this repo, most of the work was done with openzim/python-scraperlib#215 and openzim/python-scraperlib#225, and openzim/python-scraperlib@22ddefd

Note that this repo is only a very thin wrapper on top of the libzim, so the purpose is mainly to expose / document all available API(s), and explain this fact (that it's a thin wrapper ...).

Your work will mostly consist in setting up everything in this repo, I've just setup everything needed on readthedocs.org side.

@elfkuzco
Copy link

Okay. I will work on it.

@elfkuzco
Copy link

elfkuzco commented Jan 16, 2025

I have set up something that works locally. However, there are a few hiccups and I would like to know how best to proceed.
The main issue is that the mkdocstring plugin doesn't parse docstrings in .pyx files. According to the conversations in this issue, it works for compiled code (in this case libzim.so). I didn't try this though as it could mean compiling it on readthedocs or ci/cd.

On the other hand, it generates the docs for the .pyi files which is shown in the image attached below. Note that I added some docstrings in the .pyi files, otherwise it showed only class names, methods and attrs. I only did it to check things worked as I expected.

Image

Obviously, adding the docstring in all the .pyi files isn't feasible as it would mean duplication of the docstrings in the .pyx files.

So, should I stick to just working with the bare enumeration (no docstrings) of the class, method and attribute names that the .pyi files expose? Or should I explore the path of extracting the docstring from the compiled libzim.so file? What do you think?

@rgaudin
Copy link
Member

rgaudin commented Jan 16, 2025

If possible, I'd like to see how it works via the compiled lib ; just so we have all the information at hand to decide.

@elfkuzco
Copy link

If possible, I'd like to see how it works via the compiled lib ; just so we have all the information at hand to decide.

I finally set it up to work with the compiled lib but it's not quite informative.

Image
It just lists __name__ and __package__ as methods which has the same content as this comment.

I tried to use their internal tool griffe to inspect the module and see it's AST tree and it shows a tree which contains just those two attributes. I don't really know much about all the options involved with using setup.py (perhaps to add more data to the compiled binary) and it might take longer to figure out.

@rgaudin
Copy link
Member

rgaudin commented Jan 21, 2025

Thanks ; I'd like to take a quick look myself before we decide to move the dosctrings to pyi which would be suboptimal (docstrings not next to code are doomed to become incorrect overtime)

@rgaudin
Copy link
Member

rgaudin commented Jan 22, 2025

OK, I've looked at it and I think we can make it work

Image

I couldn't make it work properly though and had to hack the Python handler.

The thing is that griffe does find the proper docstrings when you pass it the -x (force inspection) flag.

❯ griffe -x libzim

The problem being that griffe is called by the python handler of mkdocstrings but never passes that information.

Here's my fix (toggling it on a new option):

*** mkdocstrings_handlers/python/handler.py.orig	2025-01-22 07:53:13
--- mkdocstrings_handlers/python/handler.py	2025-01-22 07:53:18
*************** class PythonHandler(BaseHandler):
*** 318,323 ****
--- 318,324 ----
                  modules_collection=self._modules_collection,
                  lines_collection=self._lines_collection,
                  allow_inspection=final_config["allow_inspection"],
+                 force_inspection=final_config["force_inspection"],
              )
              try:
                  for pre_loaded_module in final_config.get("preload_modules") or []:

There are ways to extend and customize with extension, plugins, etc but I find the reasonable approach is for you to fork mkdocstrings and work off your fork. Please submit them a PR with the fix, I believe it can make it and if it can't, they'll probably tell you how to do it otherwise.

Now it's not perfect and I've noted a bunch of other things to fix:

  • There are typos in docstrings
  • We use use a mix of dosctring styles. I believe we want to harmonize towards google. I fixed get_entry_by_path() in that sense.
  • @property decorated functions appear as class instead of the property's return type. that's the main issue I've found. Hopefully there's a way to fix that as well but since it's missing from the griffe output I'm not sure. I've cheated in Archive.filename so the return type is visible. If you can't fix it, please apply the same workaround and maybe open a ticket at griffe for that.
  • document (inline) all options of the mkdocs.yaml file.
  • that file also needs to be cleaned up as it's a collage of griffe's own and scraperlib's.

⚠️ you need to rebuild the wrapper (SIGN_APPLE=0 hatch run build-ext) anytime you change docstrings for it to be found by mkdocs (since it's use inspection and not reading source code). That sounds obvious but let's say I've realized that a bit late.

What I did is in the docs branch here.

@elfkuzco
Copy link

Thank you very much @rgaudin

@elfkuzco
Copy link

I have submitted a PR (mkdocstrings/python#231) and it's resolving another issue they had in their repository :).

@rgaudin
Copy link
Member

rgaudin commented Jan 24, 2025

Already merged! Awesome!

@elfkuzco
Copy link

elfkuzco commented Jan 27, 2025

Now it's not perfect and I've noted a bunch of other things to fix:

* There are typos in docstrings

* We use use a mix of dosctring styles. I believe we want to harmonize towards `google`. I fixed `get_entry_by_path()` in that sense.

* `@property` decorated functions appear as `class` instead of the property's return type. that's the main issue I've found. Hopefully there's a way to fix that as well but since it's missing from the griffe output I'm not sure. I've cheated in `Archive.filename` so the return type is visible. If you can't fix it, please apply the same workaround and maybe open a ticket at griffe for that.

* document (inline) all options of the mkdocs.yaml file.

* that file also needs to be cleaned up as it's a collage of griffe's own and scraperlib's.

I have implemented almost all of this except for a few hiccups. The first is that griffe seems to loose some of the annotation when generating the tree for compiled modules. I generated the tree for the library with and without the -x flags. Labels like property and attribute were missing when the -x flag was passed. Optionally, you can disable force_inspection and it shows the properties correctly. But then, we loose the docstrings. So, I followed the same workaround you used but I would open a ticket on it subsequently.

The second is that the mkdocs fails whenever I write docstrings for __init__ methods of classes that have a base such as FileProvider. The graph that it generates for classes like these that derive from one are cyclic. For example, there's libzim.FileProvider and libzim.writer.FileProvider. So, when it tries to generate the docstring for libzim.writer.FileProvider.__init__, that points it to libzim.FileProvider.__init__ and that in turn points to libzim.writer.FileProvider.__init__. I had to inspect the tree manually to be sure. I will most likely open a ticket on it too. My hack for it is that I omitted docstrings for classes that derive from another class.

I think all of these are consequences of inspecting a compiled module and the library probably does not handle it properly.

Aside from these two, everything is done.

@rgaudin
Copy link
Member

rgaudin commented Jan 28, 2025

OK good ; I remember there's a merge_init_into_class option on mkdocs but it looks like the problem is not with docstrings of __init__ methods but the discovery of every methods of those classes. I don't see any. WIthout inspection, I see them but obviously not the docstring…

Should you maybe start a PR with a checklist of what works and what doesn't ; with corresponding tickets (here or upstream) ? We can maybe use a progressive approach

@elfkuzco
Copy link

OK good ; I remember there's a merge_init_into_class option on mkdocs but it looks like the problem is not with docstrings of __init__ methods but the discovery of every methods of those classes. I don't see any. WIthout inspection, I see them but obviously not the docstring…

Should you maybe start a PR with a checklist of what works and what doesn't ; with corresponding tickets (here or upstream) ? We can maybe use a progressive approach

The merge_init_into_class when set to True causes the build error to happen with/without the docstring for the __init__ (with forced inspection). I had to explicitly set it to False (even though that's the default) with some comments behind the rationale in case of future edits (at least until there's a way to work around griffe). So, that only allows me to at least show the class name and some content in headings. I also have the same suspicion that it is with the discovery of methods of those classes that actually causes the error.

I think starting a PR with a checklist of what works is the best way to proceed and then work off these issues one at a time. I would need to experiment with griffe and be sure I have a better understanding of the internals before filing a ticket on their repo too.

@elfkuzco elfkuzco linked a pull request Jan 30, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants