Docs generation with pdoc

[Dliwk]

Checklist

• Remove unused code from tools/batools/docs.py (pylint is silent about this 🤔 )
• Update existing docstrings to fit new system
• Remove _ba.* function signatures from its python-docstrings.
• Add pdoc to checkenv requirements
• Move attrs documentation from class-level docstrings (need to update bastd's 'Attributes:' lines)
• pdoc#351
• Deal with generation warnings (a few remains)
• Add template macros to hide '(internal)' methods.
• Refine templates/styles a bit.
• Add some index page

Description

This PR will generate documentation using pdoc library, which (it seems) has a way more advantages than current script, at least modules other than ba is supported.

Screenshots

Type of Changes

	Type
✓	✨ New feature
✓	🔨 Refactoring
✓	📜 Docs

Closes #23

[Dliwk]

Now cpplint/pylint don't complain 🎉

[Dliwk]

I just updated dummymodule.py script, which now removes function signature lines, since signature itself is present (to avoid duplicates).

[efroemling]

The CI stuff runs tools/pcommand install_pip_reqs to get all pip stuff installed, so you probably just need to add pdoc to the list of stuff there to get CI to pass.

[Dliwk]

Hmm probably yes, I didn't do that just because I'm using a bit modified pdoc (normal pdoc sometimes hit some crazy errors while iterating over obj.__dict__, I wanted to figure this out later and possibly send them a PR). But yeah adding pdoc to that list should be fine as CI won't actually run it.

[Dliwk]

This CI fail was quite unexpected :(

[efroemling]

We have to tell mypy to not care that it has no annotations for pdoc.
Add something like the following to config/toolconfigsrc/mypy.ini and then run make prereqs

[mypy-pdoc]
ignore_missing_imports = True

[Dliwk]

We have to tell mypy to not care that it has no annotations for pdoc. Add something like the following to config/toolconfigsrc/mypy.ini and then run make prereqs

[mypy-pdoc]
ignore_missing_imports = True

Oh thanks, but I have the feeling that mypy was working a few days ago... (and this had to do something, though It looks like these guys didn't finish that).

[Dliwk]

Well, yeah, I expected something like this :( Though my pylint is fine about that.

[efroemling]

Oh thanks, but I have the feeling that mypy was working a few days ago... (and this had to do something, though It looks like these guys didn't finish that).

Hmm yeah that could have something to do with it. Though I have also found occasionally mypy gets into a weird state where it doesn't pick up certain errors until I blow away its cache. An easy way to do that is the make mypy-full target.

[Dliwk]

I'm playing with custom Jinja2 templates for pdoc right now, so would be assets/src/docs/templates a good place for them? And perhaps add this to assetstaging.py pipeline.

[Dliwk]

Pdoc can use 'attribute docstrings' as generally described in rejected PEP 224, so I decided to update existing documentation and now it should looks like:

3-space indent shouldn't be a problem as far as inspect.cleandoc handles this ok.

I still left instance-level definitions at class level just because it looks better and doesn't mess with __init__ logic.

[mhils]

Oh thanks, but I have the feeling that mypy was working a few days ago... (and this had to do something, though It looks like these guys didn't finish that).

We simply failed to include py.typed in the wheel distribution. 🤦 Which is also why it works locally but not on CI. Sorry about that, should be fixed with b51c113 in the next release! 😃

[Dliwk]

We simply failed to include py.typed in the wheel distribution. 🤦 Which is also why it works locally but not on CI. Sorry about that, should be fixed with b51c113 in the next release! 😃

That's nice 👍

[Dliwk]

Does python have some compile-time optimizers? I hit some crazy error which disappears when I'm trying to get obj.__name__ attribute in code that potentially could run but actually that never happens. 🤔

[Dliwk]

Well, I realized that pdoc gets confused with _simplify_module_names call in ba/__init__.py (and emits a huge amount of warnings). Is this necessary? Perhaps defining __all__ there should be fine.

[efroemling]

Well, I realized that pdoc gets confused with _simplify_module_names call in ba/__init__.py (and emits a huge amount of warnings). Is this necessary? Perhaps defining __all__ there should be fine.

It is simply to make names prettier (so that we get stuff like ba.Session instead of ba._session.Session). Does the __all__ mechanism do something similar?
We could probably get rid of that code if it doesn't make docs too ugly or perhaps we could conditionalize it so it doesn't run during docs generation.

[Dliwk]

Great! Fixed both cases (_ba and ba._language) 🎉

Though it doesn't work while generating documentation for another modules 🤔

(docs for bastd.actor.flag)

[mhils]

Though it doesn't work while generating documentation for another modules 🤔

Yes, that's a limitation with the current approach. If there is an alias in the current module it will be used, but it doesn't work if a module refers to another internal module that is publicly exposed only in some third module. In other words, it works if you have a single top-level namespace, but falls a bit apart otherwise. Fixing this would require pdoc to precompute all module members, aliases, and their visibility, which introduces more complexity than what I'm comfortable with for now. So I guess the best options are 1) not using _internal modules, 2) fixing it manually in Jinja2 templates, or 3) using Sphinx and then specifying things manually.

[Dliwk]

So I guess the best options are 1) not using _internal modules, 2) fixing it manually in Jinja2 templates, or 3) using Sphinx and then specifying things manually.

Or maybe try to add some overridable function (or Jinja2 macros), the result of which will also be used by qualname_candidates()?.. It seems to me clean enough.

[mhils]

Or maybe try to add some overridable function (or Jinja2 macros), the result of which will also be used by qualname_candidates()?.. It seems to me clean enough.

I'm not exactly sure if I understand what you are proposing. qualname_candidates() makes it possible that you can say Module in pdoc.doc instead of always having to specify the full pdoc.doc.Module in your docstrings. That's very similar, but I don't think that's what you are looking for.
The detection of reexposed elements is done by iterating over possible_sources() slightly below where you linked. I haven't fully thought this through, but moving the following block into a new find_alias(all_modules: dict[str, doc.Module], current_module: doc.Module, identifier: str) -> doc.Doc function that users could monkeypatch may work:
https://github.com/mitmproxy/pdoc/blob/f179a08ee170031f85b4a376e1b6471f175d0f7b/pdoc/render_helpers.py#L189-L203
You would also need to cover this part here for the link filter.
https://github.com/mitmproxy/pdoc/blob/f179a08ee170031f85b4a376e1b6471f175d0f7b/pdoc/render_helpers.py#L251-L256
Happy to review a PR, but it's not at the top of my own priority list. :)

[Dliwk]

🤔

[efroemling]

🤔

Oops; I renamed that function to 'clipboard_is_supported' and forgot to update those references. (I thought 'clipboard_available' sounded too much like it tells whether the clipboard has something on it, which is not what the function does; it simply tells whether there is clipboard functionality present at all).

Would you want to fix that in your PR? If not, I can fix it myself.

[Dliwk]

Would you want to fix that in your PR? If not, I can fix it myself.

No problem, already done. :)

[Dliwk]

*laughter* although on the other hand what else should headless build do after launch if not to bind port 😄

[efroemling]

laughter although on the other hand what else should headless build do after launch if not to bind port 😄

Yeah I figured anyone running a server would probably rather have an explicit failure in that case (instead of falling back to another port like the GUI build). But it does make it annoying when the build pipeline runs it for some data-generation purpose like that and it fails. It would probably make sense to create an environment-variable or command-line arg we could use to override that behavior.

[Dliwk]

Haha I noticed that while some things were annotated at the docstring-level, mypy didn't work with them and missed some mismatches (and now it has drawn my attention to this).

[Dliwk]

In short, ba looks nice, bastd looks quite awful.

[efroemling]

In short, ba looks nice, bastd looks quite awful.

It's ok. 'Awful' is better than nonexistent :). It'll be nice motivation for us to clean up whichever docstrings need it.

[efroemling]

Haha I noticed that while some things were annotated at the docstring-level, mypy didn't work with them and missed some mismatches (and now it has drawn my attention to this).

Interesting. I wonder if that behavior is by design or something that MyPy is overlooking. I know there are cases where I declare an attribute at the class level and don't set it at all in init and MyPy doesn't complain; perhaps this is a similar scenario.

[Dliwk]

Okay, I think it can even be merged now (I was wrong about need in python 3.10); a few remained warnings mostly is about bastd module, which can be dealt with later 🤷‍♂️

Hope I didn't accidentally blow away your changes like removal of redundant Google Play functionality or what, but it would be cool if you check. :)

[efroemling]

Okay, I think it can even be merged now (I was wrong about need in python 3.10); a few remained warnings mostly is about bastd module, which can be dealt with later 🤷‍♂️

Hope I didn't accidentally blow away your changes like removal of redundant Google Play functionality or what, but it would be cool if you check. :)

Thanks for doing this. I'm finishing up some substantial changes for 1.7, but as soon as I get to a good stopping point I'll get this stuff pulled in. Should be sometime in the next several days.

[efroemling]

Ok I'm at a good point where I can pull this in if you can clean up those few conflicts. I'm seeing one of them is with the 'all' stuff in ba/init.py. Is setting all necessary to avoid errors in documentation generation? And is there still a problem with changing module on classes?

[Dliwk]

And is there still a problem with changing module on classes?

Still the same. But I can do what I did in commit above or something similar.

Though I didn't notice the question about __all__, give me a minute.

[Dliwk]

Well, without __all__ pdoc won't handle anything, because all this located in _underscoped submodules.

[efroemling]

This all looks great. I'll go ahead and pull it in now, and I'll try to get a cron job set up to push docs to ballistica.net soon. I'm sure I'll have some feedback but I'll spend some time getting a feel for things first. Thanks so much for doing this!