Docstring review by rwb27 · Pull Request #138 · labthings/labthings-fastapi

rwb27 · 2025-07-08T09:04:14Z

In this PR, I intend to go through every function and check:

That there is a docstring
That it's formatted appropriately, using ReST
That it details all the parameters with :param <name>: syntax

I will make notes as I go in NOTES.md for my convenience. This file should be deleted before merging. Those notes will include any additions to the conceptual docs required - ideally I'd implement those as part of this PR, but if not I should at least raise an issue to note that they are missing.

I may add type hints and delete vestigial code as I go - I'll detail that in commit messages by way of explanation.

I have added most of the ruff rules in D and DOC to the section in pyproject.toml, however this doesn't properly check sphinx-style docstrings and won't verify parameters are documented. I have therefore added pydoclint and configured it to work for sphinx.

I intend in the future to make code examples in documentation testable with doctest. However, I think this probably ought to be left for a future PR. In this PR I will convert code blocks to use the ReST .. code-block:: syntax, so it will become easier to find and upgrade them to doctest-compatible ones in the future.

I will try to restrain myself from any serious refactoring or code changes, even if I spot things that I'd like to tweak. I will try to note these instead, for a future tidy-up. The occasional type hint or unused variable is, I think, fair game.

Things to check at the end:

NOTES.md is moved to an issue or this description
I reset my pre-commit hook to re-enable the linter
pydoclint is run in CI
~~[ ] This is rebased on main once Use type hints on properties instead of model argument #127 is merged~~

barecheck · 2025-07-08T09:05:18Z

Barecheck - Code coverage report

Total: 92.32%

Your code coverage diff: 0.15% ▴

Uncovered files and lines

File	Lines
src/labthings_fastapi/actions/__init__.py	183, 263-269, 387-388, 509-510, 515, 523, 548-549
src/labthings_fastapi/client/__init__.py	57, 60-61, 147-149, 159-160, 201, 212-215, 264, 307-310, 312, 314, 319-321, 323-325, 327-328, 330-334, 377
src/labthings_fastapi/client/in_server.py	120-122, 296
src/labthings_fastapi/dependencies/blocking_portal.py	45-46, 51
src/labthings_fastapi/dependencies/invocation.py	149-150
src/labthings_fastapi/dependencies/thing_server.py	64, 88
src/labthings_fastapi/descriptors/action.py	126, 130, 210-211, 215, 221, 223, 315-316, 319, 343
src/labthings_fastapi/descriptors/property.py	108, 110, 138, 140, 177-178, 343-344
src/labthings_fastapi/example_things/__init__.py	169
src/labthings_fastapi/notifications.py	18, 21, 24
src/labthings_fastapi/outputs/blob.py	92, 97, 104, 111, 137, 234, 362, 367-368, 401-402, 407, 448, 567, 643, 750
src/labthings_fastapi/outputs/mjpeg_stream.py	189, 191, 194, 197, 249-251, 260-262, 289-291, 329, 419, 456
src/labthings_fastapi/server/__init__.py	142, 168, 243, 257
src/labthings_fastapi/server/cli.py	147, 161
src/labthings_fastapi/server/fallback.py	113
src/labthings_fastapi/thing.py	240, 298, 348, 361
src/labthings_fastapi/thing_description/__init__.py	67, 76-77, 158-163, 180-184, 196, 325-326, 333
src/labthings_fastapi/thing_description/_model.py	96-97, 99
src/labthings_fastapi/thing_description/validation.py	48-49
src/labthings_fastapi/utilities/__init__.py	143, 149
src/labthings_fastapi/utilities/introspection.py	87, 97
src/labthings_fastapi/websockets.py	76

rwb27 · 2025-07-09T10:23:19Z

I've rebased on the branch #127, on the assumption that it will get merged before this one. Many thanks to @julianstirling for rebasing #127 on main. Obviously I should rebase again after #127 gets merged.

This has also included some work setting up docstring linting with `pydoclint` and tweaking `ruff` rules to flag docstring issues. Ruff does not check sphinx format docstrings very well, so I'm now using `pydoclint` directly. This should go in CI. I have retained the docstring-related Ruff rules that work, because it's useful to have them applied in pre-commit hooks.

I've turned off class attribute checks: I prefer one docstring per attribute. We may need to revisit this as per-attribute docstrings are not enforced.

I've added a `wot_td_` reference, because this needs to be referred to quite frequently.

I've added an explicit reference for "using things from other things". This may well want to be its own page in the future.

I've renamed `task` to `invocation` for consistency, and eliminated a couple of pointless functions.

I thought adding a type annotation to `Invocation.action` would be a simple improvement, but it confuses mypy badly. I have removed the annotation and the error is gone, but we should fix it properly later - I've added a note to NOTES.md. I also forgot to delete the `as_responses` argument from a call to `invocations()`, this is now fixed. Thanks for spotting that, mypy.

I've moved content from the `actions` module docstring into a page in the conceptual docs, so it can be linked to more effectively. There are a few new/improved type hints as well.

@overload

I added type information to a decorator that could be called with or without args. This confuses mypy, as I'd not added any @overload definitions to narrow down the type. I have now added the @overload definitions and it works rather better.

I've disabled this rule in favour of proper type hinting.

I have swapped a `raise` for an `assert` as I don't expect the condition should ever be false.

This also addresses an issue with pre-made dependencies, and updates the example code to use Annotated directly.

sphinx-autodoc2 gives difficult-to-follow errors that don't easily show where in the source they come from. I've set up flake8 to lint the docstrings, and fixed the errors. I've also fixed my links, which used incorrect syntax for `:ref:` links. I may try out sphinx.ext.autodoc instead - this imports the module, but might be more reliable.

Swap sphinx autodoc2 for autoapi. I've also added sphinx to dev requirements so we don't use a separate venv for docs. This may require readthedocs to be reconfigured. I've updated dev-requirements.txt to latest versions. There were a lot of ambiguous references, which I've eliminated by using a sphinx skip member function to deduplicate symbols that were re-exported (intentionally or otherwise). More use of __all__ may help here - or hiding some more modules. I've fixed up some more rst issues, too.

I also changed a couple of imports that confused autoapi - these were confusing anyway (importing external symbols from submodules).

I've excluded tests and examples from docstring lint, for now.

This also runs pydoclint as a plugin.

Self lives in typing_extensions until we stop supporting 3.10.

The CI will still test Python 3.10, but only with unpinned dependencies. This is because dependencies are usually upgraded using python >=3.11, and newer numpy versions no longer support v3.10. Keeping Python 3.10 in the unpinned-dependencies test should check it will still work, we just don't have to lock the project based on the older Python version.

`Thing._settings` should never be None, and I've relaxed typing so that `Thing` is properly recognised as an async context manager again by mypy.

rwb27 · 2025-07-17T16:12:22Z

This is frustratingly close to being ready. I think all the code changes are done - I've excluded tests from docstring linting for now, that is a future job.

Currently, flake8 runs with no errors on Python 3.12 and 3.13, but fails on Python 3.10 and 3.11. The failure is on both the Linux runner and my Windows laptop.

error traceback


(.venv_310) PS C:\Users\rbb15u\repos\labthings\labthings-fastapi> flake8 src
multiprocessing.pool.RemoteTraceback: 
"""
Traceback (most recent call last):
  File "C:\Users\rbb15u\AppData\Roaming\uv\python\cpython-3.10.18-windows-x86_64-none\lib\multiprocessing\pool.py", line 125, in worker
    result = (True, func(*args, **kwds))
  File "C:\Users\rbb15u\repos\labthings\labthings-fastapi\.venv_310\lib\site-packages\flake8\checker.py", line 82, in _mp_run
    ).run_checks()
  File "C:\Users\rbb15u\repos\labthings\labthings-fastapi\.venv_310\lib\site-packages\flake8\checker.py", line 526, in run_checks
    self.run_ast_checks()
  File "C:\Users\rbb15u\repos\labthings\labthings-fastapi\.venv_310\lib\site-packages\flake8\checker.py", line 428, in run_ast_checks
    for line_number, offset, text, _ in runner:
  File "C:\Users\rbb15u\repos\labthings\labthings-fastapi\.venv_310\lib\site-packages\flake8_docstrings.py", line 188, in run
    for error in self._check_source():
  File "C:\Users\rbb15u\repos\labthings\labthings-fastapi\.venv_310\lib\site-packages\flake8_docstrings.py", line 172, in _check_source
    for err in self._call_check_source():
  File "C:\Users\rbb15u\repos\labthings\labthings-fastapi\.venv_310\lib\site-packages\pydocstyle\checker.py", line 158, in check_source
    error = this_check(
  File "C:\Users\rbb15u\repos\labthings\labthings-fastapi\.venv_310\lib\site-packages\pydocstyle\checker.py", line 244, in check_docstring_empty
    if docstring and is_blank(ast.literal_eval(docstring)):
  File "C:\Users\rbb15u\AppData\Roaming\uv\python\cpython-3.10.18-windows-x86_64-none\lib\ast.py", line 110, in literal_eval
    return _convert(node_or_string)
  File "C:\Users\rbb15u\AppData\Roaming\uv\python\cpython-3.10.18-windows-x86_64-none\lib\ast.py", line 109, in _convert
    return _convert_signed_num(node)
  File "C:\Users\rbb15u\AppData\Roaming\uv\python\cpython-3.10.18-windows-x86_64-none\lib\ast.py", line 83, in _convert_signed_num
    return _convert_num(node)
  File "C:\Users\rbb15u\AppData\Roaming\uv\python\cpython-3.10.18-windows-x86_64-none\lib\ast.py", line 74, in _convert_num
    _raise_malformed_node(node)
  File "C:\Users\rbb15u\AppData\Roaming\uv\python\cpython-3.10.18-windows-x86_64-none\lib\ast.py", line 71, in _raise_malformed_node
    raise ValueError(msg + f': {node!r}')
ValueError: malformed node or string on line 1: <ast.JoinedStr object at 0x000001A5E0566D70>
"""

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "C:\Users\rbb15u\AppData\Roaming\uv\python\cpython-3.10.18-windows-x86_64-none\lib\runpy.py", line 196, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "C:\Users\rbb15u\AppData\Roaming\uv\python\cpython-3.10.18-windows-x86_64-none\lib\runpy.py", line 86, in _run_code
    exec(code, run_globals)
  File "C:\Users\rbb15u\repos\labthings\labthings-fastapi\.venv_310\Scripts\flake8.exe\__main__.py", line 8, in <module>
  File "C:\Users\rbb15u\repos\labthings\labthings-fastapi\.venv_310\lib\site-packages\flake8\main\cli.py", line 23, in main
    app.run(argv)
  File "C:\Users\rbb15u\repos\labthings\labthings-fastapi\.venv_310\lib\site-packages\flake8\main\application.py", line 198, in run
    self._run(argv)
  File "C:\Users\rbb15u\repos\labthings\labthings-fastapi\.venv_310\lib\site-packages\flake8\main\application.py", line 187, in _run
    self.run_checks()
  File "C:\Users\rbb15u\repos\labthings\labthings-fastapi\.venv_310\lib\site-packages\flake8\main\application.py", line 103, in run_checks
    self.file_checker_manager.run()
  File "C:\Users\rbb15u\repos\labthings\labthings-fastapi\.venv_310\lib\site-packages\flake8\checker.py", line 235, in run
    self.run_parallel()
  File "C:\Users\rbb15u\repos\labthings\labthings-fastapi\.venv_310\lib\site-packages\flake8\checker.py", line 204, in run_parallel
    self.results = list(pool.imap_unordered(_mp_run, self.filenames))
  File "C:\Users\rbb15u\AppData\Roaming\uv\python\cpython-3.10.18-windows-x86_64-none\lib\multiprocessing\pool.py", line 873, in next
    raise value
ValueError: malformed node or string on line 1: <ast.JoinedStr object at 0x000001A5E0566D70>

I'll try to debug this - I think once it is fixed this will be ready for review.

dev-dependencies.txt was regenerated under python 3.10, so numpy is no longer 2.3 and will install. I've set flake8 to only run under python 3.12 and 3.13 - it fails on older Python, but passes with no errors on 3.12 and 3.13. Given that we're linting rather than testing, I think there's little advantage to doing it four times, so I will disable that step on 3.10 and 3.11, and spend the time fixing something else.

The Sphinx error is the same as the flake8 error on Python 3.11. Changing the build version on readthedocs should fix the problem. I also now need to install the package for readthedocs (as I import it in `conf.py` to exclude some symbols). I've added `python.install.path: .` which I hope will do that.

rwb27 · 2025-07-17T22:28:44Z

Docs can be viewed at https://labthings-fastapi--138.org.readthedocs.build/en/138/

julianstirling · 2025-07-17T22:38:36Z

Thanks @rwb27, clearly a huge amount of work has gone into this and the docs on ReadTheDocs look so much more complete.

I still need to do a bit of a read through, and I am sure I will learn a lot. The two things I noticed in my very very speedy click through are:

We need a big prominent link to the ReadTheDocs in the project README, especially now so much is there.
on the deps.py CancelHook isn't documented.

rwb27 · 2025-07-17T22:45:42Z

I've updated docs/conf.py in #147 to eliminate the need to import the module. I'm not sure it's a good idea so I'll leave it there. If you agree it's not the right call, we can close that one.

rwb27 · 2025-07-17T22:57:59Z

Thanks for spotting CancelHook. Not sure how I missed it! I guess the linter doesn't enforce docstrings on variables, unfortunately there's not much I can do about that - I've enabled all the docstring-related rules I could find...

julianstirling

This is an incredible amount of work thank you @rwb27.

I have done a high level walk through of this. I think we won't truly know what else needs to be done with the docs until we start using them in anger.

I've spotted a couple of little things. I think also probably clearest to remove the examples directory if it is empty.

README.md

examples/README.md

docs/source/using_things.rst

rwb27 · 2025-07-18T13:34:03Z

Thanks for that - typos are fixed and examples folder is gone. I put an examples page in the docs with links, I thought it would be helpful to point people to examples, but it's the right call to lose the examples folder.

I'm under no illusion that the docs are now finished, but it's a significant step in the right direction and hopefully we'll continue to build the higher level docs pages as we go.

rwb27 marked this pull request as draft July 8, 2025 09:04

rwb27 mentioned this pull request Jul 8, 2025

Use type hints on properties instead of model argument #127

Closed

rwb27 force-pushed the docstring-review branch from 908f9de to 7a26827 Compare July 9, 2025 10:53

rwb27 added 15 commits July 9, 2025 16:36

Add notes file (to be removed before merging)

5d55fff

Check docstrings on invocation_model

3bf9bee

I've turned off class attribute checks: I prefer one docstring per attribute. We may need to revisit this as per-attribute docstrings are not enforced.

Improved docstrings on client module

edf83b4

I've added a `wot_td_` reference, because this needs to be referred to quite frequently.

Switch sphinx to rst mode again in docstrings

8cbb4ff

Docstring improvements in DirectThingClient

a193f40

I've added an explicit reference for "using things from other things". This may well want to be its own page in the future.

Improve docstrings and naming in client code

d24920e

I've renamed `task` to `invocation` for consistency, and eliminated a couple of pointless functions.

Whitespace fixes

057ca01

Fix a D401 from ruff

64cad0c

Update lt_core_concepts to use newly-defined links.

1deda39

Improve docstrings in descriptors and add conceptual docs on actions

92d8a40

I've moved content from the `actions` module docstring into a page in the conceptual docs, so it can be linked to more effectively. There are a few new/improved type hints as well.

Fix ambiguous type hint

81460f5

I added type information to a decorator that could be called with or without args. This confuses mypy, as I'd not added any @overload definitions to narrow down the type. I have now added the @overload definitions and it works rather better.

Module level docstring for dependencies

a6824ad

Docstring improvements in dependencies/action_manager

f5dee80

rwb27 force-pushed the docstring-review branch from d3c10aa to f5dee80 Compare July 9, 2025 15:37

rwb27 added 9 commits July 9, 2025 16:53

Don't put yield types in docstrings

f7f2761

I've disabled this rule in favour of proper type hinting.

Improvements to blocking portal docstrings

cd73933

Started work on invocation dependencies.

407e562

Link to conceptual docs and tidy exception.

436d9ac

I have swapped a `raise` for an `assert` as I don't expect the condition should ever be false.

Finished documenting invocation deps.

8530ea2

Finish docstringing the dependencies

274a685

Improve conceptual docs

288aa66

This also addresses an issue with pre-made dependencies, and updates the example code to use Annotated directly.

Note the change to how I recommend using DirectThingClient.

ecdc274

Docstring improvements to ActionDescriptor

e473dab

rwb27 added 10 commits July 16, 2025 22:38

Fixed the last few reference/rst issues

b445f09

I also changed a couple of imports that confused autoapi - these were confusing anyway (importing external symbols from submodules).

Update readthedocs

d462d2b

Get ruff and flake8 passing.

d226bdc

I've excluded tests and examples from docstring lint, for now.

Add flake8 to CI and readme

cc5af86

This also runs pydoclint as a plugin.

Standardise on :return: and :raise X:

e9ed606

fix typing import for python 3.10

4c6cc4a

Self lives in typing_extensions until we stop supporting 3.10.

Typing fixes

7d16af6

`Thing._settings` should never be None, and I've relaxed typing so that `Thing` is properly recognised as an async context manager again by mypy.

rwb27 added 2 commits July 17, 2025 21:15

rwb27 force-pushed the docstring-review branch from ed19085 to 060b963 Compare July 17, 2025 22:21

rwb27 marked this pull request as ready for review July 17, 2025 22:28

Add comments to docs/conf.py and test.yml

cd636f9

Delete NOTES, update README, and add a final docstring

72fa52e

Spotted a couple of typos/RST issues

2a4ef7e

julianstirling mentioned this pull request Jul 18, 2025

Use Doctest to test examples in the documentation #148

Open

julianstirling reviewed Jul 18, 2025

View reviewed changes

README.md Outdated Show resolved Hide resolved

examples/README.md Outdated Show resolved Hide resolved

docs/source/using_things.rst Outdated Show resolved Hide resolved

Fix typos and delete examples folder in response to review.

0f47e7f

rwb27 requested a review from julianstirling July 18, 2025 13:34

julianstirling approved these changes Jul 18, 2025

View reviewed changes

rwb27 merged commit d2d5fe6 into main Jul 18, 2025
14 checks passed

rwb27 deleted the docstring-review branch July 18, 2025 16:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Docstring review#138

Docstring review#138
rwb27 merged 58 commits intomainfrom
docstring-review

rwb27 commented Jul 8, 2025 •

edited by julianstirling

Loading

Uh oh!

barecheck bot commented Jul 8, 2025 •

edited

Loading

Uh oh!

rwb27 commented Jul 9, 2025

Uh oh!

rwb27 commented Jul 17, 2025

Uh oh!

rwb27 commented Jul 17, 2025

Uh oh!

julianstirling commented Jul 17, 2025

Uh oh!

rwb27 commented Jul 17, 2025 •

edited

Loading

Uh oh!

rwb27 commented Jul 17, 2025

Uh oh!

julianstirling left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rwb27 commented Jul 18, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

rwb27 commented Jul 8, 2025 • edited by julianstirling Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

barecheck bot commented Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Barecheck - Code coverage report

Uh oh!

rwb27 commented Jul 9, 2025

Uh oh!

rwb27 commented Jul 17, 2025

Uh oh!

rwb27 commented Jul 17, 2025

Uh oh!

julianstirling commented Jul 17, 2025

Uh oh!

rwb27 commented Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rwb27 commented Jul 17, 2025

Uh oh!

julianstirling left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rwb27 commented Jul 18, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rwb27 commented Jul 8, 2025 •

edited by julianstirling

Loading

barecheck bot commented Jul 8, 2025 •

edited

Loading

rwb27 commented Jul 17, 2025 •

edited

Loading