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

docs: expand the reference documentation #169

Merged
merged 18 commits into from
Aug 30, 2024

Conversation

tonyandrewmeyer
Copy link
Collaborator

@tonyandrewmeyer tonyandrewmeyer commented Aug 11, 2024

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.

Copy link
Collaborator

@PietroPasotti PietroPasotti left a comment

Choose a reason for hiding this comment

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

Looks good to me!

scenario/state.py Outdated Show resolved Hide resolved
Co-authored-by: PietroPasotti <[email protected]>
@tonyandrewmeyer
Copy link
Collaborator Author

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

Copy link
Member

@tmihoc tmihoc left a comment

Choose a reason for hiding this comment

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

At first pass, this is all very nice. Left some comments on the "unit testing" classification and on terminology. Plus we should sync with Charmcraft and make sure ops.scenario is featured in their profiles.

scenario/__init__.py Show resolved Hide resolved

"""Charm state-transition testing SDK for Operator Framework charms.

Write unit tests that declaratively define the Juju state all at once, define
Copy link
Member

Choose a reason for hiding this comment

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

So we promote scenario as another way to do unit tests, then? We should sync with Charmcraft to make sure their profiles create a directory structure for tests that is aligned.

Copy link
Collaborator Author

@tonyandrewmeyer tonyandrewmeyer Aug 29, 2024

Choose a reason for hiding this comment

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

We're (imminently) going to be promoting Scenario as the preferred way to do unit tests. I also tried to have these work if you were reading them when you'd done pip install ops-scenario but also if you had done pip install ops[testing] and were getting all the classes from the ops.testing namespace. This is also why all the references to the name "scenario" in the docs are gone.

I do have a ticket for updating the charmcraft profiles. I need to get all the bits in places first, but it's definitely in my definition-of-done.

scenario/__init__.py Outdated Show resolved Hide resolved
scenario/__init__.py Show resolved Hide resolved
scenario/__init__.py Outdated Show resolved Hide resolved
scenario/consistency_checker.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@dimaqq dimaqq left a comment

Choose a reason for hiding this comment

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

Looks good after a fast end-to-end read.

@tonyandrewmeyer tonyandrewmeyer marked this pull request as ready for review August 29, 2024 09:28
@tonyandrewmeyer
Copy link
Collaborator Author

@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
Copy link
Collaborator

benhoyt commented Aug 29, 2024

@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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

@benhoyt what about this version?

@benhoyt
Copy link
Collaborator

benhoyt commented Aug 30, 2024

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
Copy link
Collaborator Author

(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
Copy link
Collaborator Author

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.

@tonyandrewmeyer tonyandrewmeyer merged commit dfdce85 into canonical:7.0 Aug 30, 2024
2 checks passed
@tonyandrewmeyer tonyandrewmeyer deleted the expand-api-docs branch August 30, 2024 04:46
tonyandrewmeyer added a commit that referenced this pull request Sep 2, 2024
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.

---------

Co-authored-by: PietroPasotti <[email protected]>
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.

5 participants