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

Decide on docstring format #755

Closed
coretl opened this issue Feb 5, 2025 · 4 comments · Fixed by #764
Closed

Decide on docstring format #755

coretl opened this issue Feb 5, 2025 · 4 comments · Fixed by #764

Comments

@coretl
Copy link
Collaborator

coretl commented Feb 5, 2025

After spending 2 weeks with sphinx autodoc and autosummary I have failed to get it to document typevars properly, which means a bunch of unclickable links in the docs everytime you write SignalDatatypeT. This is annoying as it happens in hundreds of places, and the user generally would like to know at this point "which datatypes could I pass here?" so really needs that link.

To fix, we could:

  1. Manually link to the datatypes documentation in every docstring
  2. Fix autodoc
  3. Switch to sphinx-autodoc2

I think 1. is too manual and error prone, I failed at 2. after 2 days of hacking, so I suggest 3.

It is almost working, but there is one decision to make, what format should the docstrings be?

Given that prose documents are written in markdown and look like this:

# Connecting the Device
Rather than calling [](#Device.connect) yourself which would use the wrong event loop you can 
use [](#init_devices) at startup or the equivalent [plan stub](#ensure_connected). Remember to 
pass `mock=True` during testing.

We can either write the docstring in markdown or RST, and we can format the arguments using param list, google style or numpy style. I've included a sample docstring below that we can comment on:

Numpy style with RST links

    def create_devices_from_annotations(
        self,
        filled=True,
    ) -> Iterator[tuple[DeviceConnectorT, list[Any]]]:
        """Create all Signals from annotations

        Parameters
        ----------
        filled
            If ``True`` then the Devices created should be considered already filled
            with connection data. If ``False`` then `fill_child_device` needs
            calling at parent device connection time before the child `Device` can
            be connected.

        Yields
        ------
        (connector, extras):
            The `DeviceConnector` that has been created for this Signal, and the list of
            extra annotations that could be used to customize it.
        """

Google style with RST links

    def create_devices_from_annotations(
        self,
        filled=True,
    ) -> Iterator[tuple[DeviceConnectorT, list[Any]]]:
        """Create all Signals from annotations

        Args:
            filled: If ``True`` then the Devices created should be considered
                already filled with connection data. If ``False`` then
                `fill_child_device` needs calling at parent device connection
                time before the child `Device` can be connected.

        Yields:
            (connector, extras): The `DeviceConnector` that has been created for this
                Signal, and the list of extra annotations that could be used to
                customize it.
        """

Param list with markdown links

    def create_devices_from_annotations(
        self,
        filled=True,
    ) -> Iterator[tuple[DeviceConnectorT, list[Any]]]:
        """Create all Signals from annotations

        :param filled: If `True` then the Devices created should be considered
            already filled with connection data. If `False` then
            [](#fill_child_device) needs calling at parent device connection
            time before the child [](#Device) can be connected.

        :yields: `(connector, extras)`: the [](#DeviceConnector) that has been
            created for this Signal, and the list of extra annotations that
            could be used to customize it.
        """

Or any combination of the above.

Acceptance Criteria

  • Decision is made and documented in an ADR
@coretl
Copy link
Collaborator Author

coretl commented Feb 5, 2025

I am leaning towards markdown links because then you only have to remember one way of cross linking, and param list because it is already supported in autodoc2 without additional work

@stan-dot
Copy link
Contributor

stan-dot commented Feb 5, 2025

document typevars properly, which means a bunch of unclickable links in the docs everytime you write SignalDatatypeT.

example: https://github.com/bluesky/ophyd-async/blob/main/src/ophyd_async/core/_mock_signal_backend.py

this isn't as highly maintained
https://github.com/sphinx-extensions2/sphinx-autodoc2

there are other popular alternatives, most of which use markdown
https://www.gitbook.com/

@coretl
Copy link
Collaborator Author

coretl commented Feb 5, 2025

We aren't switching away from sphinx, that's too big a jump. The question is which docstring rendering plugin within sphinx to use. sphinx-autodoc2 seems recommended by myst, it's written by a maintainer of sphinx, but hasn't attracted any other contributors yet. It is however a small codebase, and actually extensible in the application, so we can prototype much of what we want without needing a new release of it.

@coretl coretl mentioned this issue Feb 10, 2025
@coretl
Copy link
Collaborator Author

coretl commented Feb 11, 2025

In the absence of any other feedback I'll go with Param list with markdown links

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 a pull request may close this issue.

2 participants