Stricter ruff

.github/workflows/test.yml

@@ -74,10 +74,7 @@ jobs:
           path: ./coverage.lcov
 
       - name: Analyse with MyPy
-        run: mypy src
-
-      - name: Type tests with MyPy
-        run: mypy --warn-unused-ignores typing_tests
+        run: mypy
 
   test-with-unpinned-deps:
     runs-on: ubuntu-latest
@@ -105,7 +102,7 @@ jobs:
 
       - name: Analyse with MyPy
         if: success() || failure()
-        run: mypy src
+        run: mypy
 
       - name: Test with pytest
         if: success() || failure()

dev-requirements.txt

@@ -55,6 +55,10 @@ email-validator==2.2.0
     # via
     #   fastapi
     #   pydantic
+exceptiongroup==1.3.0
+    # via
+    #   anyio
+    #   pytest
 fastapi==0.116.1
     # via labthings-fastapi (pyproject.toml)
 fastapi-cli==0.0.8
@@ -160,9 +164,10 @@ pyflakes==3.4.0
 pygments==2.19.2
     # via
     #   flake8-rst-docstrings
+    #   pytest
     #   rich
     #   sphinx
-pytest==7.4.4
+pytest==8.4.1
     # via
     #   labthings-fastapi (pyproject.toml)
     #   pytest-cov
@@ -241,6 +246,14 @@ sphinxcontrib-serializinghtml==2.0.0
     # via sphinx
 starlette==0.47.1
     # via fastapi
+tomli==2.2.1
+    # via
+    #   coverage
+    #   flake8-pyproject
+    #   mypy
+    #   pydoclint
+    #   pytest
+    #   sphinx
 typer==0.16.0
     # via
     #   fastapi-cli
@@ -251,16 +264,20 @@ typing-extensions==4.14.1
     # via
     #   labthings-fastapi (pyproject.toml)
     #   anyio
+    #   astroid
+    #   exceptiongroup
     #   fastapi
     #   mypy
     #   pydantic
     #   pydantic-core
     #   pydantic-extra-types
     #   referencing
+    #   rich
     #   rich-toolkit
     #   starlette
     #   typer
     #   typing-inspection
+    #   uvicorn
 typing-inspection==0.4.1
     # via pydantic-settings
 ujson==5.10.0

docs/source/quickstart/counter.py

@@ -1,13 +1,15 @@
+"""An example Thing that implements a counter."""
+
 import time
 import labthings_fastapi as lt
 
 
 class TestThing(lt.Thing):
-    """A test thing with a counter property and a couple of actions"""
+    """A test thing with a counter property and a couple of actions."""
 
     @lt.thing_action
     def increment_counter(self) -> None:
-        """Increment the counter property
+        """Increment the counter property.
 
         This action doesn't do very much - all it does, in fact,
         is increment the counter (which may be read using the
@@ -17,13 +19,13 @@ def increment_counter(self) -> None:
 
     @lt.thing_action
     def slowly_increase_counter(self) -> None:
-        """Increment the counter slowly over a minute"""
+        """Increment the counter slowly over a minute."""
         for _i in range(60):
             time.sleep(1)
             self.increment_counter()
 
     counter: int = lt.property(default=0, readonly=True)
-    "A pointless counter"
+    "A pointless counter."
 
 
 if __name__ == "__main__":

docs/source/quickstart/counter_client.py

@@ -1,3 +1,5 @@
+"""Client code that interacts with the counter Thing over HTTP."""
+
 from labthings_fastapi import ThingClient
 
 counter = ThingClient.from_url("http://localhost:5000/counter/")

pyproject.toml

@@ -25,7 +25,7 @@ dependencies = [
 
 [project.optional-dependencies]
 dev = [
-  "pytest>=7.4.0, <8",
+  "pytest>=8.4.0",
   "pytest-cov",
   "pytest-mock",
   "mypy>=1.6.1, <2",
@@ -77,7 +77,20 @@ docstring-code-format = true
 
 [tool.ruff.lint]
 external = ["DOC401", "F824", "DOC101", "DOC103"]  # used via flake8/pydoclint
-select = ["B", "E4", "E7", "E9", "F", "D", "DOC"]
+select = [
+  "ANN",   # type annotations (ensuring they are present, complements mypy)
+  "ASYNC", # flake8 async rules
+  "B",     # flake8-bugbear
+  "BLE",   # don't catch Exception
+  "C4",    # flake8-comprehension rules
+  "E",     # flake8
+  "F",     # flake8
+  "FAST",  # FastAPI rules (mostly around dependencies)
+  "D",     # pydoclint (recommended by the developers over flake8 plugin)
+  "DOC",   # pydocstyle (limited support for sphinx style: see ignores)
+  "FAST",  # FastAPI rules
+  "S",     # flake8-bandit - notably, this disallows `assert`
+]
 ignore = [
     "D203",  # incompatible with D204
     "D213",  # incompatible with D212
@@ -88,16 +101,24 @@ ignore = [
     "B008",  # This disallows function calls in default values.
     # FastAPI Depends() breaks this rule, and FastAPI's response is "disable it".
     # see https://github.com/fastapi/fastapi/issues/1522
+    "ANN401",  # ANN401 disallows Any. There are quite a few places where Any is
+    # needed for dynamically typed code.
 ]
 preview = true
 
 [tool.ruff.lint.per-file-ignores]
 # Tests are currently not fully docstring-ed, we'll ignore this for now.
-"tests/*" = ["D", "DOC"]
+"tests/*" = [
+    "D",      # Tests are not yet fully covered by docstrings
+    "DOC",    # Tests are not yet fully covered by docstrings
+    "ANN",    # We don't require type hints in test code.
+    "S101",   # S101 disallows assert. That's not appropriate for tests.
+]
 # Typing tests do have docstrings, but it's not helpful to insist on imperative
 # mood etc.
 "typing_tests/*" = ["D404", "D401"]
-"docs/*" = ["D", "DOC"]
+# The docs config file is a python file, but doesn't conform to the usual rules.
+"docs/source/conf.py" = ["D", "DOC", "ANN"]
 
 [tool.ruff.lint.pydocstyle]
 # This lets the D401 checker understand that decorated thing properties and thing
@@ -108,7 +129,12 @@ property-decorators = [
 ]
 
 [tool.mypy]
+files = ["src/", "typing_tests/"]
 plugins = ["pydantic.mypy"]
+disallow_untyped_defs = true
+check_untyped_defs = true
+disallow_untyped_decorators = true
+warn_unused_ignores = true
 
 
 [tool.flake8]

src/labthings_fastapi/actions/__init__.py

@@ -78,7 +78,7 @@
         dependencies: Optional[dict[str, Any]] = None,
         log_len: int = 1000,
         cancel_hook: Optional[CancelHook] = None,
-    ):
+    ) -> None:
         """Create a thread to run an action and track its outputs.
 
         :param action: provides the function that we run, as well as metadata
@@ -142,8 +142,8 @@
         """
         try:
             blobdata_to_url_ctx.get()
         except LookupError as e:
             raise NoBlobManagerError(
                 "An invocation output has been requested from a api route that "
                 "doesn't have a BlobIOContextDep dependency. This dependency is needed "
                 " for blobs to identify their url."
@@ -168,17 +168,31 @@
             return self._status
 
     @property
-    def action(self):
-        """The `.ActionDescriptor` object running in this thread."""
+    def action(self) -> ActionDescriptor:
+        """The `.ActionDescriptor` object running in this thread.
+
+        :raises RuntimeError: if the action descriptor has been deleted.
+            This should never happen, as the descriptor is a property of
+            a class, which won't be deleted.
+        """
         action = self.action_ref()
-        assert action is not None, "The action for an `Invocation` has been deleted!"
+        if action is None:  # pragma: no cover
+            # Action descriptors should only be deleted after the server has
+            # stopped, so this error should never occur.
+            raise RuntimeError("The action for an `Invocation` has been deleted!")
         return action
 
     @property
     def thing(self) -> Thing:
-        """The `.Thing` to which the action is bound, i.e. this is ``self``."""
+        """The `.Thing` to which the action is bound, i.e. this is ``self``.
+
+        :raises RuntimeError: if the Thing no longer exists.
+        """
         thing = self.thing_ref()
-        assert thing is not None, "The `Thing` on which an action was run is missing!"
+        if thing is None:  # pragma: no cover
+            # this error block is primarily for mypy: the Thing will exist as
+            # long as the server is running, so we should never hit this error.
+            raise RuntimeError("The `Thing` on which an action was run is missing!")
         return thing
 
     def cancel(self) -> None:
@@ -210,10 +224,12 @@
             LinkElement(rel="self", href=href),
             LinkElement(rel="output", href=href + "/output"),
         ]
-        return self.action.invocation_model(
+        # The line below confuses MyPy because self.action **evaluates to** a Descriptor
+        # object (i.e. we don't call __get__ on the descriptor).
+        return self.action.invocation_model(  # type: ignore[call-overload]
             status=self.status,
             id=self.id,
-            action=self.thing.path + self.action.name,
+            action=self.thing.path + self.action.name,  # type: ignore[call-overload]
             href=href,
             timeStarted=self._start_time,
             timeCompleted=self._end_time,
@@ -248,38 +264,45 @@
         stored. The status is then set to ERROR and the thread terminates.
 
         See `.Invocation.status` for status values.
+
+        :raises RuntimeError: if there is no Thing associated with the invocation.
         """
+        # self.action evaluates to an ActionDescriptor. This confuses mypy,
+        # which thinks we are calling ActionDescriptor.__get__.
+        action: ActionDescriptor = self.action  # type: ignore[call-overload]
         try:
-            self.action.emit_changed_event(self.thing, self._status)
+            action.emit_changed_event(self.thing, self._status.value)
 
             # Capture just this thread's log messages
             handler = DequeLogHandler(dest=self._log)
             logger = invocation_logger(self.id)
             logger.addHandler(handler)
 
-            action = self.action
             thing = self.thing
             kwargs = model_to_dict(self.input)
-            assert action is not None
-            assert thing is not None
+            if thing is None:  # pragma: no cover
+                # The Thing is stored as a weakref, but it will always exist
+                # while the server is running - this error should never
+                # occur.
+                raise RuntimeError("Cannot start an invocation without a Thing.")
 
             with self._status_lock:
                 self._status = InvocationStatus.RUNNING
                 self._start_time = datetime.datetime.now()
-                self.action.emit_changed_event(self.thing, self._status)
+                action.emit_changed_event(self.thing, self._status.value)
 
             # The next line actually runs the action.
             ret = action.__get__(thing)(**kwargs, **self.dependencies)
 
             with self._status_lock:
                 self._return_value = ret
                 self._status = InvocationStatus.COMPLETED
-                self.action.emit_changed_event(self.thing, self._status)
+                action.emit_changed_event(self.thing, self._status.value)
         except InvocationCancelledError:
             logger.info(f"Invocation {self.id} was cancelled.")
             with self._status_lock:
                 self._status = InvocationStatus.CANCELLED
-                self.action.emit_changed_event(self.thing, self._status)
+                action.emit_changed_event(self.thing, self._status.value)
         except Exception as e:  # skipcq: PYL-W0703
             # First log
             if isinstance(e, InvocationError):
@@ -291,7 +314,7 @@
             with self._status_lock:
                 self._status = InvocationStatus.ERROR
                 self._exception = e
-                self.action.emit_changed_event(self.thing, self._status)
+                action.emit_changed_event(self.thing, self._status.value)
         finally:
             with self._status_lock:
                 self._end_time = datetime.datetime.now()
@@ -309,7 +332,7 @@
         self,
         dest: MutableSequence,
         level: int = logging.INFO,
-    ):
+    ) -> None:
         """Set up a log handler that appends messages to a deque.
 
         .. warning::
@@ -341,9 +364,9 @@
 class ActionManager:
     """A class to manage a collection of actions."""
 
-    def __init__(self):
+    def __init__(self) -> None:
         """Set up an `.ActionManager`."""
-        self._invocations = {}
+        self._invocations: dict[uuid.UUID, Invocation] = {}
         self._invocations_lock = Lock()
 
     @property
@@ -409,8 +432,8 @@
         :param id: the unique ID of the action to retrieve.
         :return: the `.Invocation` object.
         """
         with self._invocations_lock:
             return self._invocations[id]
 
     def list_invocations(
         self,
@@ -443,10 +466,13 @@
             i.response(request=request)
             for i in self.invocations
             if thing is None or i.thing == thing
-            if action is None or i.action == action
+            if action is None or i.action == action  # type: ignore[call-overload]
+            # i.action evaluates to an ActionDescriptor, which confuses mypy - it
+            # thinks we are calling ActionDescriptor.__get__ but this isn't ever
+            # called.
         ]
 
-    def expire_invocations(self):
+    def expire_invocations(self) -> None:
         """Delete invocations that have passed their expiry time."""
         to_delete = []
         with self._invocations_lock:
@@ -465,7 +491,9 @@
         """
 
         @app.get(ACTION_INVOCATIONS_PATH, response_model=list[InvocationModel])
-        def list_all_invocations(request: Request, _blob_manager: BlobIOContextDep):
+        def list_all_invocations(
+            request: Request, _blob_manager: BlobIOContextDep
+        ) -> list[InvocationModel]:
             return self.list_invocations(request=request)
 
         @app.get(
@@ -512,7 +540,9 @@
                 503: {"description": "No result is available for this invocation"},
             },
         )
-        def action_invocation_output(id: uuid.UUID, _blob_manager: BlobIOContextDep):
+        def action_invocation_output(
+            id: uuid.UUID, _blob_manager: BlobIOContextDep
+        ) -> Any:
             """Get the output of an action invocation.
 
             This returns just the "output" component of the action invocation. If the
@@ -531,8 +561,8 @@
             with self._invocations_lock:
                 try:
                     invocation: Any = self._invocations[id]
                 except KeyError as e:
                     raise HTTPException(
                         status_code=404,
                         detail="No action invocation found with ID {id}",
                     ) from e
@@ -545,7 +575,7 @@
                     invocation.output.response
                 ):
                     # TODO: honour "accept" header
                     return invocation.output.response()
                 return invocation.output
 
         @app.delete(
@@ -570,8 +600,8 @@
             with self._invocations_lock:
                 try:
                     invocation: Any = self._invocations[id]
                 except KeyError as e:
                     raise HTTPException(
                         status_code=404,
                         detail="No action invocation found with ID {id}",
                     ) from e