Stricter mypy

.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()

pyproject.toml

@@ -108,7 +108,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

@@ -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,7 +168,7 @@
             return self._status
 
     @property
-    def action(self):
+    def action(self) -> ActionDescriptor:
         """The `.ActionDescriptor` object running in this thread."""
         action = self.action_ref()
         assert action is not None, "The action for an `Invocation` has been deleted!"
@@ -210,10 +210,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,
@@ -249,37 +251,38 @@
 
         See `.Invocation.status` for status values.
         """
+        # 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
 
             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 +294,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()
@@ -341,9 +344,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 +412,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 +446,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 +471,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 +520,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,13 +541,13 @@
             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
                 if not invocation.output:
                     raise HTTPException(
                         status_code=503,
                         detail="No result is available for this invocation",
                     )
@@ -545,7 +555,7 @@
                     invocation.output.response
                 ):
                     # TODO: honour "accept" header
                     return invocation.output.response()
                 return invocation.output
 
         @app.delete(
@@ -570,8 +580,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

src/labthings_fastapi/client/__init__.py

@@ -54,11 +54,11 @@
     :raise KeyError: if there is no link with the specified ``rel`` value.
     """
     if "links" not in obj:
         raise ObjectHasNoLinksError(f"Can't find any links on {obj}.")
     try:
         return next(link for link in obj["links"] if link["rel"] == rel)
     except StopIteration as e:
         raise KeyError(f"No link was found with rel='{rel}' on {obj}.") from e
 
 
 def invocation_href(invocation: dict) -> str:
@@ -144,11 +144,11 @@
 
         :return: the property's value, as deserialised from JSON.
         """
         r = self.client.get(urljoin(self.path, path))
         r.raise_for_status()
         return r.json()
 
-    def set_property(self, path: str, value: Any):
+    def set_property(self, path: str, value: Any) -> None:
         """Make a PUT request to set the value of a property.
 
         :param path: the URI of the ``getproperty`` endpoint, relative
@@ -156,10 +156,10 @@
         :param value: the property's value. Currently this must be
             serialisable to JSON.
         """
         r = self.client.put(urljoin(self.path, path), json=value)
         r.raise_for_status()
 
-    def invoke_action(self, path: str, **kwargs):
+    def invoke_action(self, path: str, **kwargs: Any) -> Any:
         r"""Invoke an action on the Thing.
 
         This method will make the initial POST request to invoke an action,
@@ -180,8 +180,17 @@
         :raise RuntimeError: is raised if the action does not complete successfully.
         """
         for k in kwargs.keys():
-            if isinstance(kwargs[k], ClientBlobOutput):
-                kwargs[k] = {"href": kwargs[k].href, "media_type": kwargs[k].media_type}
+            value = kwargs[k]
+            if isinstance(value, ClientBlobOutput):
+                # ClientBlobOutput objects may be used as input to a subsequent
+                # action. When this is done, they should be serialised to a dict
+                # with `href` and `media_type` keys, as done below.
+                # Ideally this should be replaced with `Blob` and the use of
+                # `pydantic` models to serialise action inputs.
+                #
+                # Note that the blob will not be uploaded: we rely on the blob
+                # still existing on the server.
+                kwargs[k] = {"href": value.href, "media_type": value.media_type}
         r = self.client.post(urljoin(self.path, path), json=kwargs)
         r.raise_for_status()
         invocation = poll_invocation(self.client, r.json())
@@ -270,7 +279,9 @@
 class PropertyClientDescriptor:
     """A base class for properties on `.ThingClient` objects."""
 
-    pass
+    name: str
+    type: type | BaseModel
+    path: str
 
 
 def property_descriptor(
@@ -312,10 +323,10 @@
     if readable:
 
         def __get__(
-            self,
+            self: PropertyClientDescriptor,
             obj: Optional[ThingClient] = None,
             _objtype: Optional[type[ThingClient]] = None,
-        ):
+        ) -> Any:
             if obj is None:
                 return self
             return obj.get_property(self.name)
@@ -324,7 +335,9 @@
         P.__get__ = __get__  # type: ignore[attr-defined]
     if writeable:
 
-        def __set__(self, obj: ThingClient, value: Any):
+        def __set__(
+            self: PropertyClientDescriptor, obj: ThingClient, value: Any
+        ) -> None:
             obj.set_property(self.name, value)
 
         __set__.__annotations__["value"] = model
@@ -349,7 +362,7 @@
         format.
     """
 
-    def action_method(self, **kwargs):
+    def action_method(self: ThingClient, **kwargs: Any) -> Any:
         return self.invoke_action(action_name, **kwargs)
 
     if "output" in action and "type" in action["output"]:
@@ -359,7 +372,7 @@
     setattr(cls, action_name, action_method)
 
 
-def add_property(cls: type[ThingClient], property_name: str, property: dict):
+def add_property(cls: type[ThingClient], property_name: str, property: dict) -> None:
     """Add a property to a ThingClient subclass.
 
     A descriptor will be added to the provided class that makes the

src/labthings_fastapi/client/in_server.py

@@ -113,18 +113,22 @@ class P(PropertyClientDescriptor):
         path = property_path or property_name
 
     def __get__(
-        self,
+        self: PropertyClientDescriptor,
         obj: Optional[DirectThingClient] = None,
         _objtype: Optional[type[DirectThingClient]] = None,
-    ):
+    ) -> Any:
         if obj is None:
             return self
         return getattr(obj._wrapped_thing, self.name)
 
-    def __set__(self, obj: DirectThingClient, value: Any):
+    def __set__(
+        self: PropertyClientDescriptor, obj: DirectThingClient, value: Any
+    ) -> None:
         setattr(obj._wrapped_thing, self.name, value)
 
-    def set_readonly(self, obj: DirectThingClient, value: Any):
+    def set_readonly(
+        self: PropertyClientDescriptor, obj: DirectThingClient, value: Any
+    ) -> None:
         raise AttributeError("This property is read-only.")
 
     if readable:
@@ -198,7 +202,7 @@ def add_action(
     """
 
     @wraps(action.func)
-    def action_method(self, **kwargs):
+    def action_method(self: DirectThingClient, **kwargs: Any) -> Any:
         dependency_kwargs = {
             param.name: self._dependencies[param.name]
             for param in action.dependency_params
@@ -273,7 +277,7 @@ def direct_thing_client_class(
 
     def init_proxy(
         self: DirectThingClient, request: Request, **dependencies: Mapping[str, Any]
-    ):
+    ) -> None:
         r"""Initialise a DirectThingClient (this docstring will be replaced).
 
         :param self: The DirectThingClient instance we're initialising.

src/labthings_fastapi/decorators/__init__.py

@@ -37,15 +37,15 @@
 """
 
 from functools import wraps, partial
-from typing import Optional, Callable, overload
+from typing import Any, Optional, Callable, overload
 from ..descriptors import (
     ActionDescriptor,
     EndpointDescriptor,
     HTTPMethod,
 )
 
 
-def mark_thing_action(func: Callable, **kwargs) -> ActionDescriptor:
+def mark_thing_action(func: Callable, **kwargs: Any) -> ActionDescriptor:
     r"""Mark a method of a Thing as an Action.
 
     We replace the function with a descriptor that's a
@@ -65,12 +65,12 @@ class ActionDescriptorSubclass(ActionDescriptor):
 
 
 @overload
-def thing_action(func: Callable, **kwargs) -> ActionDescriptor: ...
+def thing_action(func: Callable, **kwargs: Any) -> ActionDescriptor: ...
 
 
 @overload
 def thing_action(
-    **kwargs,
+    **kwargs: Any,
 ) -> Callable[
     [
         Callable,
@@ -81,7 +81,7 @@ def thing_action(
 
 @wraps(mark_thing_action)
 def thing_action(
-    func: Optional[Callable] = None, **kwargs
+    func: Optional[Callable] = None, **kwargs: Any
 ) -> (
     ActionDescriptor
     | Callable[
@@ -121,7 +121,7 @@ def thing_action(
 
 
 def fastapi_endpoint(
-    method: HTTPMethod, path: Optional[str] = None, **kwargs
+    method: HTTPMethod, path: Optional[str] = None, **kwargs: Any
 ) -> Callable[[Callable], EndpointDescriptor]:
     r"""Mark a function as a FastAPI endpoint without making it an action.
 

src/labthings_fastapi/dependencies/invocation.py

@@ -146,7 +146,7 @@ def __init__(self, id: InvocationID):
         threading.Event.__init__(self)
         self.invocation_id = id
 
-    def raise_if_set(self):
+    def raise_if_set(self) -> None:
         """Raise an exception if the event is set.
 
         This is intended as a compact alternative to:
@@ -161,7 +161,7 @@ def raise_if_set(self):
         if self.is_set():
             raise InvocationCancelledError("The action was cancelled.")
 
-    def sleep(self, timeout: float):
+    def sleep(self, timeout: float) -> None:
         r"""Sleep for a given time in seconds, but raise an exception if cancelled.
 
         This function can be used in place of `time.sleep`. It will usually behave