search

canonical / ops-scenario

State-transition testing SDK for Operator Framework Juju charms.
Apache License 2.0
10 stars 7 forks source link

docs: expand the reference documentation #169

Closed tonyandrewmeyer closed 2 weeks ago

tonyandrewmeyer commented 1 month ago

Expands the docstrings so that when used with Sphinx's autodoc they will fit in with the other ops docs on ops.rtd but also still work standalone.

The Sphinx autodoc system uses the __new__ signature in preference to the __init__ one, which means that by default all classes that are using the _MaxPositionalArgs class have a *args, **kwargs signature, which is not informative. custom_conf.py monkeypatches Sphinx to work around this, including tweaking the signature so that the * appears in the correct place for the maximum number of positional arguments.

Also bumps the Sphinx version to align with ops.

tonyandrewmeyer commented 1 month ago

Hi @tmihoc ! Would you mind giving this a review when you have some time? Thanks!

tonyandrewmeyer commented 2 weeks ago

@PietroPasotti, @benhoyt, or @dimaqq would one of you mind having a quick look over my signature hack to make sure I haven't done anything stupid? No need to re-review all the rest of the changes.

You can get the output with tox -e docs to verify that it's showing what it should.

benhoyt commented 2 weeks ago

@PietroPasotti, @benhoyt, or @dimaqq would one of you mind having a quick look over my signature hack to make sure I haven't done anything stupid? No need to re-review all the rest of the changes.

You can get the output with tox -e docs to verify that it's showing what it should.

Hmmm, it's a cool hack, but I don't love it as code in our repo. :-) I wonder if we can hack or monkey-patch Sphinx instead of cluttering the main codebase?

tonyandrewmeyer commented 2 weeks ago

Hmmm, it's a cool hack, but I don't love it as code in our repo. :-)

It does make the signature arguably more correct - I feel like the real hack is the _MaxPositionalArgs class. It would also only be there until we can move to Python 3.10+ and go back to plain dataclasses.

I wonder if we can hack or monkey-patch Sphinx instead of cluttering the main codebase?

This should work in theory. I'll work on that.

tonyandrewmeyer commented 2 weeks ago

@benhoyt what about this version?

benhoyt commented 2 weeks ago

Yeah, it's not too bad -- I prefer that over there than polluting the main codebase. (Side note: isn't that going to recurse, calling sphinx.ext.autodoc.ClassDocumenter._get_signature() once we've replaced it with our custom version? Wouldn't you need to grab the old version first, then call that?)

But let's have a brief discussion during/after daily to finalise this.

tonyandrewmeyer commented 2 weeks ago

(Side note: isn't that going to recurse, calling sphinx.ext.autodoc.ClassDocumenter._get_signature() once we've replaced it with our custom version? Wouldn't you need to grab the old version first, then call that?)

Huh, indeed it ought to. And yet it works somehow. But I'll fix that.

tonyandrewmeyer commented 2 weeks ago

But let's have a brief discussion during/after daily to finalise this.

Had a chat about this - it seems like the two best options (until we can move to Python 3.10) are what's here now or going back to manually crafted __init__ methods, which is a lot more invasive and will be more work to pull out once we can.

Ben's ok with going ahead with this method.