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

Extract and dynamically re-inject base client sphinx param doc into client classes, for better sphinx and runtime doc #1131

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sirosen
Copy link
Member

@sirosen sirosen commented Jan 18, 2025

Add three helpers to utils in order to improve the runtime
docstrings for client classes.

  1. read_sphinx_params -- a simple :param: parser
  2. inject_sphinx_params -- a __doc__ rewriter which takes parsed
    params and inserts them in place of a special marker comment
  3. inject_sphinx_params_of -- a decorator version of these which
    chains together reading the docstring of one object and
    modifying the docstring of another

Then apply these to write

@utils.inject_sphinx_params_of(client.BaseClient)
class FlowsClient(client.BaseClient):
    ...

and so forth. This is applied to all client classes except for the
Auth login clients, which do not inherit all of the parameters of
BaseClient -- in particular, we do not allow construction of a
login client with an app.

read_sphinx_params gets a functools cache, but the rest of the work
is still done dynamically, so there is a little bit of repetition.

This results in improved docstrings for sphinx and for runtime usages
like help(globus_sdk.TransferClient). Static analyzers (e.g., IDEs)
will pick up on the original source string, which will show the marker
comment used to drive this behavior:

.. globus-sdk-inject-doc-params

before after
image image

📚 Documentation preview 📚: https://globus-sdk-python--1131.org.readthedocs.build/en/1131/

Add three helpers to `utils` in order to improve the runtime
docstrings for client classes.

1. `read_sphinx_params` -- a simple `:param:` parser
2. `inject_sphinx_params` -- a `__doc__` rewriter which takes parsed
        params and inserts them in place of a special marker comment
3. `inject_sphinx_params_of` -- a decorator version of these which
        chains together reading the docstring of one object and
        modifying the docstring of another

Then apply these to write

    ```python
    @utils.inject_sphinx_params_of(client.BaseClient)
    class FlowsClient(client.BaseClient):
        ...
    ```

and so forth. This is applied to all client classes except for the
Auth login clients, which do not inherit all of the parameters of
`BaseClient` -- in particular, we do not allow construction of a
login client with an `app`.

`read_sphinx_params` gets a functools cache, but the rest of the work
is still done dynamically, so there is a little bit of repetition.

This results in improved docstrings for sphinx and for runtime usages
like `help(globus_sdk.TransferClient)`. Static analyzers (e.g., IDEs)
will pick up on the original source string, which will show the marker
comment used to drive this behavior:

    ```
    .. globus-sdk-inject-doc-params
    ```
@sirosen sirosen force-pushed the better-client-param-doc branch from 70d79d6 to 722071f Compare January 18, 2025 23:51
Comment on lines +17 to 18
@utils.inject_sphinx_params_of(client.BaseClient)
class SearchClient(client.BaseClient):
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to do this without the annotation by looking at the client class's mro instead of annotating?

current: list[str] = []
for line in docstring.splitlines():
if not current:
if line.startswith(":param"):
Copy link
Contributor

Choose a reason for hiding this comment

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

Surely sphinx loads these as a part of its docstring parsing these. Do we have any mechanism to tap into that in a plugin instead of doing it ourselves?

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.

2 participants