From c84a950bab2dfeeb5b3c58e863825b903642cd40 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Thu, 24 Jul 2025 21:32:29 +0100 Subject: [PATCH 01/59] Add documentation pages on OpenAPI/TD and Descriptors --- docs/source/documentation.rst | 16 ++++++++++++++++ docs/source/index.rst | 1 + docs/source/see_also.rst | 21 +++++++++++++++++++++ 3 files changed, 38 insertions(+) create mode 100644 docs/source/documentation.rst create mode 100644 docs/source/see_also.rst diff --git a/docs/source/documentation.rst b/docs/source/documentation.rst new file mode 100644 index 00000000..6ea4a322 --- /dev/null +++ b/docs/source/documentation.rst @@ -0,0 +1,16 @@ +.. _gen_docs: + +Generated documentation +======================= + +LabThings describes its HTTP API in two ways: with a :ref:`wot_td` and with an OpenAPI_ document. + +OpenAPI_ is a standard way to describe an HTTP interface. It lists all of the possible HTTP requests that may be made, along with a description of each one, and a description of the possible responses. + +Each :ref:`wot_thing` is documented by a Thing Description, which is a JSON document describing all of the ways to interact with that Thing. The WoT_ standard defines the `Thing Description`_ and includes a JSON Schema against which it may be validated. + +Thing Description documents are higher-level than OpenAPI_ and focus on the capabilities of the Thing, rather than the HTTP endpoints. In general you would expect more HTTP endpoints than there are interaction affordances, so in principle client code based on a Thing Description should be more meaningful. However, OpenAPI is a much more widely adopted standard and so both forms of documentation are generated by LabThings-FastAPI. + +.. _WoT: https://www.w3.org/WoT/ +.. _Thing Description: https://www.w3.org/TR/wot-thing-description/ +.. _OpenAPI: https://www.openapis.org/ diff --git a/docs/source/index.rst b/docs/source/index.rst index 2a38b82e..81c9301d 100644 --- a/docs/source/index.rst +++ b/docs/source/index.rst @@ -15,6 +15,7 @@ Documentation for LabThings-FastAPI blobs.rst concurrency.rst using_things.rst + see_also.rst autoapi/index diff --git a/docs/source/see_also.rst b/docs/source/see_also.rst new file mode 100644 index 00000000..dc5781c5 --- /dev/null +++ b/docs/source/see_also.rst @@ -0,0 +1,21 @@ +See Also +======== + +LabThings-FastAPI makes quite heavy use of a few key concepts from external libraries, including `fastapi`, `pydantic`, and of course Python's core library. This page attempts to summarise these, and also acts as a useful place for docstrings to link to, so we can avoid repetition. + +.. _descriptors: + +Descriptors +----------- + +Descriptors are a way to itercept attribute access on an object. By default, attributes of an object are just variables - so an object called ``foo`` might have an attribute called ``bar``, and you may read its value with ``foo.bar``, write its value with ``foo.bar = "baz"``, and delete the attribute with ``del foo.bar``. If ``foo`` is a descriptor, Python will call the ``__get__`` method of that descriptor when it's read and the ``__set__`` method when it's written to. The descriptor protocol is described with plenty of examples in the `Descriptor Guide`_ in the Python documentation. + +In LabThings-FastAPI, descriptors are used to implement :ref:`wot_actions` and :ref:`wot_properties` on `.Thing` subclasses. The intention is that these will function like standard Python methods and properties, but will also be available over HTTP, along with automatic documentation in the :ref:`wot_td` and OpenAPI documents. + +There are a few useful notes that relate to many of the descriptors in LabThings-FastAPI: + +* Descriptor objects **may have more than one owner**. As a rule, a descriptor object (e.g. an instance of `.DataProperty`) is assigned to an attribute of one `.Thing` subclass. There may, however, be multiple *instances* of that class, so it is not safe to assume that the descriptor object corresponds to only one `.Thing`. This is why the `.Thing` is passed to the ``__get__`` method: we should ensure that any values being remembered are keyed to the owning `.Thing` and are not simply stored in the descriptor. Usually, this is done using `.WeakKeyDictionary` objects, which allow us to look up values based on the `.Thing`, without interfering with garbage collection. +* Descriptor objects **may know their name**. Python calls ``__set_name__`` on a descriptor if it is available. This allows the descriptor to know the name of the attribute to which it is assigned. LabThings-FastAPI uses the name in the URL and in the Thing Description. When ``__set_name__`` is called, the descriptor **is also passed the class that owns it**. This allows us to check for type hints and docstrings that are part of the class, rather than part of the descriptor. +* There is a convention that descriptors return their value when accessed as an instance attribute, but return themselves when accessed as a class attribute. We try to adhere to that convention. + +.. _`Descriptor Guide`: https://docs.python.org/3/howto/descriptor.html \ No newline at end of file From a914ac2c786129813d5ebd0a516a1a12ab26ae3a Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Thu, 24 Jul 2025 22:06:49 +0100 Subject: [PATCH 02/59] First pass at splitting up ThingProperty This commit swaps ThingProperty for two descriptor classes, FunctionalProperty (using getters/setters) and DataProperty (variable-like). This preserves the two useful behaviours out of the six (!) possible behaviours implemented by the old ThingProperty. I've also added a `property` function that provides a single interface to both: it may be used either as a decorator, or as a field specifier. This does not yet pass tests/type checking. --- pyproject.toml | 11 +- src/labthings_fastapi/__init__.py | 4 +- src/labthings_fastapi/base_descriptor.py | 173 +++++ src/labthings_fastapi/descriptors/property.py | 402 ---------- src/labthings_fastapi/exceptions.py | 41 +- src/labthings_fastapi/thing_property.py | 696 ++++++++++++++++++ 6 files changed, 916 insertions(+), 411 deletions(-) create mode 100644 src/labthings_fastapi/base_descriptor.py delete mode 100644 src/labthings_fastapi/descriptors/property.py create mode 100644 src/labthings_fastapi/thing_property.py diff --git a/pyproject.toml b/pyproject.toml index e0b0758a..d6c0e96d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -76,14 +76,15 @@ target-version = "py310" docstring-code-format = true [tool.ruff.lint] +external = ["DOC401", "F824"] # used via flake8/pydoclint select = ["E4", "E7", "E9", "F", "D", "DOC"] ignore = [ "D203", # incompatible with D204 "D213", # incompatible with D212 - "DOC402", # doesn't work with sphinx-style docstrings, use pydoclint - "DOC201", # doesn't work with sphinx-style docstrings, use pydoclint - "DOC501", # doesn't work with sphinx-style docstrings, use pydoclint - "DOC502", # doesn't work with sphinx-style docstrings, use pydoclint + "DOC402", # doesn't work with sphinx-style docstrings, use flake8/pydoclint + "DOC201", # doesn't work with sphinx-style docstrings, use flake8/pydoclint + "DOC501", # doesn't work with sphinx-style docstrings, use flake8/pydoclint + "DOC502", # doesn't work with sphinx-style docstrings, use flake8/pydoclint ] preview = true @@ -98,6 +99,8 @@ plugins = ["pydantic.mypy", "numpy.typing.mypy_plugin"] extend-ignore = [ "DOC301", # allow class + __init__ docstrings "D202", # conflicts with ruff format + "F401", # already implemented by ruff, which respects "import x as x" + "E501", # leave this to ruff (line length), #noqa may exceed max line length. ] max-line-length = 88 rst-roles = [ diff --git a/src/labthings_fastapi/__init__.py b/src/labthings_fastapi/__init__.py index ec969f65..49128591 100644 --- a/src/labthings_fastapi/__init__.py +++ b/src/labthings_fastapi/__init__.py @@ -20,7 +20,7 @@ """ from .thing import Thing -from .descriptors import ThingProperty, ThingSetting +from .thing_property import property, ThingSetting from .decorators import ( thing_property, thing_setting, @@ -42,7 +42,7 @@ # re-export style, we may switch in the future. __all__ = [ "Thing", - "ThingProperty", + "property", "ThingSetting", "thing_property", "thing_setting", diff --git a/src/labthings_fastapi/base_descriptor.py b/src/labthings_fastapi/base_descriptor.py new file mode 100644 index 00000000..815f186c --- /dev/null +++ b/src/labthings_fastapi/base_descriptor.py @@ -0,0 +1,173 @@ +"""A base class for descriptors in LabThings. + +:ref:`descriptors` are used to describe :ref:`wot_affordances` in LabThings-FastAPI. +There is some behaviour common to most of these, and `.BaseDescriptor` centralises +the code that implements it. +""" + +from typing import overload, Generic, Self, TypeVar, TYPE_CHECKING + +from .utilities.introspection import get_summary + +if TYPE_CHECKING: + from .thing import Thing + +Value = TypeVar("Value") +"""The value returned by the descriptor, when called on an instance.""" + + +class NameNotSetError(AttributeError): + """Descriptor name is not yet available. + + This error is raised if the name of an affordance is accessed + before ``__set_name__`` has been called on the descriptor. + """ + + +class BaseDescriptor(Generic[Value]): + r"""A base class for descriptors in LabThings-FastAPI. + + This class implements several behaviours common to descriptors in LabThings: + + * The descriptor remembers the name it's assigned to in ``name``, for use in + :ref:`gen_docs`\ . + * When called as a class attribute, the descriptor returns itself, as done by + e.g. `property`. + """ + + def __init__(self): + """Initialise a BaseDescriptor.""" + self._name: str | None = None + self._title: str | None = None + self._description: str | None = None + # We set the instance __doc__ to None so the descriptor class docstring + # doesn't get picked up by OpenAPI/Thing Description. + self.__doc__ = None + + def __set_name__(self, owner: type[Thing], name: str) -> None: + r"""Take note of the name to which the descriptor is assigned. + + This is called when the descriptor is assigned to an attribute of a class. + This function just remembers the name, so it can be used in + :ref:`gen_docs`\ . + + :param owner: the `.Thing` subclass to which we are being attached. + :param name: the name to which we have been assigned. + """ + # Remember the name to which we're assigned. Accessed by the read only + # property ``name``. + self._name = name + + @property + def name(self) -> str: + """The name of this descriptor. + + When the descriptor is assigned to an attribute of a class, we + remember the name of the attribute. There will be some time in + between the descriptor being instantiated and the name being set. + If this is accessed before we know the name, we will raise an exception. + + The ``name`` of :ref:`wot_affordances` is used in their URL and in + the :ref:`gen_docs` served by LabThings. + + :raises NameNotSetError: if it is accessed before the descriptor + has been notified of its name. + """ + if self._name is None: + raise NameNotSetError + return self._name + + @property + def title(self) -> str: + """A human-readable title for the descriptor. + + The :ref:`wot_td` requires a human-readable title for all + :ref:`wot_affordances` described. This property will generate a + suitable string from either the name or the docstring. + + The title is either the first line of the docstring, or the name + of the descriptor. Note that, if there's no summary line in the + descriptor's instance docstring, or if ``__set__name__`` has not + yet been called (i.e. if this attribute is accessed before the + class on which the descriptor is defined has been fully set up), + the `.NameNotSetError` from ``self.name`` will propagate, i.e. + this property will either return a string or fail with an + exception. + + Note also that, if the docstring for this descriptor is defined + on the class rather than passed in (via a getter function or + action function's docstring), it will also not be available until + after ``__set_name__`` has been called. + """ + if not self._title: + # First, try to retrieve the first line of the docstring. + # This is the preferred option for the title. + self._title = get_summary(self) + if not self._title: + # If there's no docstring, or it doesn't have a summary line, + # use the name of the descriptor instead. + # Note that this will either succeed or raise an exception. + self._title = self.name + return self._title + + @property + def description(self) -> str | None: + """A description of the descriptor for use in documentation. + + This property will return the docstring describing the descriptor. + As the first line of the docstring (if present) is used as the + ``title`` in :ref:`gen_docs` it will be removed from this property. + """ + + # I have ignored D105 (missing docstrings) on the overloads - these should not + # exist on @overload definitions. + @overload + def __get__(self, obj: Thing, type: type | None = None) -> Value: ... # noqa: D105 + + @overload + def __get__(self, type: type) -> Self: ... # noqa: D105 + + def __get__(self, obj: Thing | None, type: type | None = None) -> Value | Self: + """Return the value or the descriptor, as per `property`. + + If ``obj`` is ``None`` (i.e. the descriptor is accessed as a class attribute), + we return the descriptor, i.e. ``self``. + + If ``obj`` is not ``None``, we return a value. To remove the need for this + boilerplate in every subclass, we will call ``__instance_get__`` to get the + value. + + :param obj: the `.Thing` instance to which we are attached. + :param type: the `.Thing` subclass on which we are defined. + + :return: the value of the descriptor returned from ``__instance_get__`` when + accessed on an instance, or the descriptor object if accessed on a class. + """ + if obj is not None: + return self.instance_get(obj, type) + return self + + def instance_get(self, obj: Thing) -> Value: + """Return the value of the descriptor. + + This method is called from ``__get__`` if the descriptor is accessed as an + instance attribute. This means that ``obj`` is guaranteed to be present. + + ``__get__`` may be called on either an instance or a class, and if it is + called on the class, the convention is that we should return the descriptor + object (i.e. ``self``), as done by `builtins.property`. + + `.BaseDescriptor.__get__` takes care of this logic, so we need only consider + the case where we are called as an instance attribute. This simplifies type + annotations and removes the need for overload definitions in every subclass. + + :param obj: is the `.Thing` instance on which this descriptor is being + accessed. + :return: the value of the descriptor (i.e. property value, or bound method). + + :raises NotImplementedError: if it is not overridden. + """ + raise NotImplementedError( + "__instance_get__ must be defined on BaseDescriptor subclasses. \n\n" + "See BaseDescriptor.__instance_get__ for details." + ) diff --git a/src/labthings_fastapi/descriptors/property.py b/src/labthings_fastapi/descriptors/property.py deleted file mode 100644 index d877f33b..00000000 --- a/src/labthings_fastapi/descriptors/property.py +++ /dev/null @@ -1,402 +0,0 @@ -"""Define a descriptor to represent properties. - -:ref:`wot_properties` are represented in LabThings by `.ThingProperty` descriptors. -These descriptors work similarly to regular Python properties or attributes, -with the addition of features that allow them to be accessed over HTTP and -documented in the :ref:`wot_td` and OpenAPI documents. - -This module defines the `.ThingProperty` class. -""" - -from __future__ import annotations -from typing import TYPE_CHECKING, Annotated, Any, Callable, Optional -from weakref import WeakSet - -from typing_extensions import Self -from pydantic import BaseModel, RootModel -from fastapi import Body, FastAPI - -from ..utilities import labthings_data, wrap_plain_types_in_rootmodel -from ..utilities.introspection import get_summary, get_docstring -from ..thing_description._model import PropertyAffordance, Form, DataSchema, PropertyOp -from ..thing_description import type_to_dataschema -from ..exceptions import NotConnectedToServerError - - -if TYPE_CHECKING: - from ..thing import Thing - - -class ThingProperty: - """A property that can be accessed via the HTTP API. - - By default, a ThingProperty acts like - a normal variable, but functionality can be added in several ways. - """ - - model: type[BaseModel] - readonly: bool = False - - def __init__( - self, - model: type, - initial_value: Any = None, - readonly: bool = False, - observable: bool = False, - description: Optional[str] = None, - title: Optional[str] = None, - getter: Optional[Callable] = None, - setter: Optional[Callable] = None, - ): - """Create a property that can be accessed via the HTTP API. - - `.ThingProperty` is a descriptor that functions like a variable, optionally - with notifications when it is set. It may also have a getter and setter, - which work in a similar way to Python properties. - - `.ThingProperty` can behave in several different ways: - - * If no ``getter`` or ``setter`` is specified, it will behave like a simple - data attribute (i.e. a variable). If ``observable`` is ``True``, it is - possible to register for notifications when the value is set. In this - case, an ``initial_value`` is required. - * If a ``getter`` is specified and ``observable`` is ``False``, the ``getter`` - will be called when the property is accessed, and its return value - will be the property's value, just like the builtin ``property``. The - property will be read-only both locally and via HTTP. - * If a ``getter`` is specified and ``observable`` is ``True``, the ``getter`` - is used instead of ``initial_value`` but thereafter the property - behaves like a variable. The ``getter`` is only on first access. - The property may be written to locally, and whether it's writable - via HTTP depends on the ``readonly`` argument. - * If both a ``getter`` and ``setter`` are specified and ``observable`` is - ``False``, - the property behaves like a Python property, with the ``getter`` being - called when the property is accessed, and the ``setter`` being called - when the property is set. The property is read-only via HTTP if - ``readonly`` is ``True``. It may always be written to locally. - * If ``observable`` is ``True`` and a ``setter`` is specified, the property - will behave like a variable, but will call the ``setter`` - when the property is set. The ``setter`` may perform tasks like sending - the updated value to the hardware, but it is not responsible for - remembering the value. The initial value is set via the ``getter`` or - ``initial_value``. - - - :param model: The type of the property. This is optional, because it is - better to use type hints (see notes on typing above). - :param initial_value: The initial value of the property. If this is set, - the property must not have a getter, and should behave like a variable. - :param readonly: If True, the property cannot be set via the HTTP API. - :param observable: If True, the property can be observed for changes via - websockets. This causes the setter to run code in the async event loop - that will notify a list of subscribers each time the property is set. - Currently, only websockets can be used to observe properties. - :param description: A description of the property, used in the API - documentation. LabThings will attempt to take this from the docstring - if not supplied. - :param title: A human-readable title for the property, used in the API - documentation. Defaults to the first line of the docstring, or the name - of the property. - :param getter: A function that gets the value of the property. - :param setter: A function that sets the value of the property. - - :raise ValueError: if the initial value or type are missing or incorrectly - specified. - """ - if getter and initial_value is not None: - raise ValueError("getter and an initial value are mutually exclusive.") - if model is None: - raise ValueError("LabThings Properties must have a type") - self.model = wrap_plain_types_in_rootmodel(model) - self.readonly = readonly - self.observable = observable - self.initial_value = initial_value - self._description = description - self._title = title - # The lines below allow _getter and _setter to be specified by subclasses - self._setter = setter or getattr(self, "_setter", None) - self._getter = getter or getattr(self, "_getter", None) - # Try to generate a DataSchema, so that we can raise an error that's easy to - # link to the offending ThingProperty - type_to_dataschema(self.model) - - def __set_name__(self, owner: type[Thing], name: str) -> None: - """Take note of the name to which the descriptor is assigned. - - This is called when the descriptor is assigned to an attribute of a class. - - :param owner: the `.Thing` subclass to which we are being attached. - :param name: the name to which we have been assigned. - """ - self._name = name - - @property - def title(self): - """A human-readable title for the property.""" - if self._title: - return self._title - if self._getter and get_summary(self._getter): - return get_summary(self._getter) - return self.name - - @property - def description(self): - """A description of the property.""" - return self._description or get_docstring(self._getter, remove_summary=True) - - def __get__(self, obj: Thing | None, type: type | None = None) -> Any: - """Return the value of the property. - - If `obj` is none (i.e. we are getting the attribute of the class), - we return the descriptor. - - If no getter is set, we'll return either the initial value, or the value - from the object's __dict__, i.e. we behave like a variable. - - If a getter is set, we will use it, unless the property is observable, at - which point the getter is only ever used once, to set the initial value. - - :param obj: the `.Thing` to which we are attached. - :param type: the class on which we are defined. - - :return: the value of the property (when accessed on an instance), or - this descriptor if accessed as a class attribute. - """ - if obj is None: - return self - try: - if self._getter and not self.observable: - # if there's a getter and the property isn't observable, use it - return self._getter(obj) - # otherwise, behave like a variable and return our value - return obj.__dict__[self.name] - except KeyError: - if self._getter: - # if we get to here, the property should be observable, so cache - obj.__dict__[self.name] = self._getter(obj) - return obj.__dict__[self.name] - else: - return self.initial_value - - def __set__(self, obj: Thing, value: Any) -> None: - """Set the property's value. - - :param obj: the `.Thing` to which we are attached. - :param value: the new value for the property. - """ - obj.__dict__[self.name] = value - if self._setter: - self._setter(obj, value) - self.emit_changed_event(obj, value) - - def _observers_set(self, obj: Thing): - """Return the observers of this property. - - Each observer in this set will be notified when the property is changed. - See ``.ThingProperty.emit_changed_event`` - - :param obj: the `.Thing` to which we are attached. - - :return: the set of observers corresponding to ``obj``. - """ - ld = labthings_data(obj) - if self.name not in ld.property_observers: - ld.property_observers[self.name] = WeakSet() - return ld.property_observers[self.name] - - def emit_changed_event(self, obj: Thing, value: Any) -> None: - """Notify subscribers that the property has changed. - - This function is run when properties are updated. It must be run from - within a thread. This could be the `Invocation` thread of a running action, or - the property should be updated over via a client/http. It must be run from a - thread as it is communicating with the event loop via an `asyncio` blocking - portal and can cause deadlock if run in the event loop. - - :param obj: the `.Thing` to which we are attached. - :param value: the new property value, to be sent to observers. - - :raise NotConnectedToServerError: if the Thing that is calling the property - update is not connected to a server with a running event loop. - """ - runner = obj._labthings_blocking_portal - if not runner: - thing_name = obj.__class__.__name__ - msg = ( - f"Cannot emit property updated changed event. Is {thing_name} " - "connected to a running server?" - ) - raise NotConnectedToServerError(msg) - runner.start_task_soon( - self.emit_changed_event_async, - obj, - value, - ) - - async def emit_changed_event_async(self, obj: Thing, value: Any): - """Notify subscribers that the property has changed. - - This function may only be run in the `anyio` event loop. See - `.ThingProperty.emit_changed_event`. - - :param obj: the `.Thing` to which we are attached. - :param value: the new property value, to be sent to observers. - """ - for observer in self._observers_set(obj): - await observer.send( - {"messageType": "propertyStatus", "data": {self._name: value}} - ) - - @property - def name(self): - """The name of the property. - - This should be consistent between the class definition and the - :ref:`wot_td` as well as appearing in the URLs for getting and setting. - """ - return self._name - - def add_to_fastapi(self, app: FastAPI, thing: Thing) -> None: - """Add this action to a FastAPI app, bound to a particular Thing. - - :param app: The FastAPI application we are adding endpoints to. - :param thing: The `.Thing` we are adding the endpoints for. - """ - # We can't use the decorator in the usual way, because we'd need to - # annotate the type of `body` with `self.model` which is only defined - # at runtime. - # The solution below is to manually add the annotation, before passing - # the function to the decorator. - if not self.readonly: - - def set_property(body): # We'll annotate body later - if isinstance(body, RootModel): - body = body.root - return self.__set__(thing, body) - - set_property.__annotations__["body"] = Annotated[self.model, Body()] - app.put( - thing.path + self.name, - status_code=201, - response_description="Property set successfully", - summary=f"Set {self.title}", - description=f"## {self.title}\n\n{self.description or ''}", - )(set_property) - - @app.get( - thing.path + self.name, - response_model=self.model, - response_description=f"Value of {self.name}", - summary=self.title, - description=f"## {self.title}\n\n{self.description or ''}", - ) - def get_property(): - return self.__get__(thing) - - def property_affordance( - self, thing: Thing, path: Optional[str] = None - ) -> PropertyAffordance: - """Represent the property in a Thing Description. - - :param thing: the `.Thing` to which we are attached. - :param path: the URL of the `.Thing`. If not present, we will retrieve - the ``path`` from ``thing``. - - :return: A description of the property in :ref:`wot_td` format. - """ - path = path or thing.path - ops = [PropertyOp.readproperty] - if not self.readonly: - ops.append(PropertyOp.writeproperty) - forms = [ - Form[PropertyOp]( - href=path + self.name, - op=ops, - ), - ] - data_schema: DataSchema = type_to_dataschema(self.model) - pa: PropertyAffordance = PropertyAffordance( - title=self.title, - forms=forms, - description=self.description, - ) - # We merge the data schema with the property affordance (which subclasses the - # DataSchema model) with the affordance second so its values take priority. - # Note that this works because all of the fields that get filled in by - # DataSchema are optional - so the PropertyAffordance is still valid without - # them. - return PropertyAffordance( - **{ - **data_schema.model_dump(exclude_none=True), - **pa.model_dump(exclude_none=True), - } - ) - - def getter(self, func: Callable) -> Self: - """Set the function that gets the property's value. - - :param func: is the new getter function. - - :return: this property (to allow its use as a decorator). - """ - self._getter = func - return self - - def setter(self, func: Callable) -> Self: - """Change the setter function. - - `.ThingProperty` descriptors return the value they hold - when they are accessed. However, they can run code when they are set: this - decorator sets a function as that code. - - :param func: is the new setter function. - - :return: this property (to allow its use as a decorator). - """ - self._setter = func - self.readonly = False - return self - - -class ThingSetting(ThingProperty): - """A `.ThingProperty` that persists on disk. - - A setting can be accessed via the HTTP API and is persistent between sessions. - - A `.ThingSetting` is a `.ThingProperty` with extra functionality for triggering - a `.Thing` to save its settings. - - Note: If a setting is mutated rather than assigned to, this will not trigger saving. - For example: if a Thing has a setting called `dictsetting` holding the dictionary - `{"a": 1, "b": 2}` then `self.dictsetting = {"a": 2, "b": 2}` would trigger saving - but `self.dictsetting[a] = 2` would not, as the setter for `dictsetting` is never - called. - - The setting otherwise acts just like a normal variable. - """ - - def __set__(self, obj: Thing, value: Any): - """Set the setting's value. - - This will cause the settings to be saved to disk. - - :param obj: the `.Thing` to which we are attached. - :param value: the new value of the setting. - """ - super().__set__(obj, value) - obj.save_settings() - - def set_without_emit(self, obj: Thing, value: Any): - """Set the property's value, but do not emit event to notify the server. - - This function is not expected to be used externally. It is called during - initial setup so that the setting can be set from disk before the server - is fully started. - - :param obj: the `.Thing` to which we are attached. - :param value: the new value of the setting. - """ - obj.__dict__[self.name] = value - if self._setter: - self._setter(obj, value) diff --git a/src/labthings_fastapi/exceptions.py b/src/labthings_fastapi/exceptions.py index 3fa595ae..786fe14a 100644 --- a/src/labthings_fastapi/exceptions.py +++ b/src/labthings_fastapi/exceptions.py @@ -1,9 +1,39 @@ """A submodule for custom LabThings-FastAPI Exceptions.""" -from .dependencies.invocation import InvocationCancelledError +import inspect +# The "import x as x" syntax means symbols are interpreted as being re-exported, +# so they won't be flagged as unused by the linter. +# An __all__ for this module is less than helpful, unless we have an +# automated check that everything's included. +from .dependencies.invocation import ( + InvocationCancelledError as InvocationCancelledError, +) -class NotConnectedToServerError(RuntimeError): + +class DocstringToMessage: + """A mixin to put Exception docstrings in as their default message.""" + + append_to_message: bool = True + + def __init__(self, message: str | None): + """Initialise an error with a message or its docstring. + + :param message: the optional message. + """ + doc = inspect.cleandoc(self.__doc__) if self.__doc__ else None + if message: + if doc and self.append_to_message: + super().__init__(message + "\n\n" + doc) + else: + super().__init__(message) + elif doc: + super().__init__(doc) + else: + super().__init__() + + +class NotConnectedToServerError(DocstringToMessage, RuntimeError): """The Thing is not connected to a server. This exception is called if a ThingAction is called or @@ -13,4 +43,9 @@ class NotConnectedToServerError(RuntimeError): """ -__all__ = ["NotConnectedToServerError", "InvocationCancelledError"] +class ReadOnlyPropertyError(DocstringToMessage, AttributeError): + """A property is read-only. + + No setter has been defined for this `.FunctionalProperty`, so + it may not be written to. + """ diff --git a/src/labthings_fastapi/thing_property.py b/src/labthings_fastapi/thing_property.py new file mode 100644 index 00000000..b1d3a1bf --- /dev/null +++ b/src/labthings_fastapi/thing_property.py @@ -0,0 +1,696 @@ +"""Define properties of `.Thing` objects. + +:ref:`wot_properties` are attributes of a `.Thing` that may be read or written to +over HTTP, and they are described in :ref:`gen_docs`. They are implemented with +a function `.property` (usually referenced as ``lt.property``), which is +intentionally similar to Python's built in `property`. + +Properties can be defined in two ways as shown below: + +.. code-block:: python + + import labthings_fastapi as lt + + class Counter(lt.Thing): + "A counter that knows what's remaining." + + count: int = lt.property(default=0, readonly=True) + "The number of times we've increnented the counter." + + target: int = lt.property(default=10) + "The number of times to increment before we stop." + + @lt.property + def remaining(self) -> int: + "The number of steps remaining." + return self.remaining - self.count + + @remaining.setter + def remaining(self, value: int) -> None: + self.target = self.count + value + + The first two properties are simple variables: they may be read and assigned + to, and will behave just like a regular variable. Their syntax is similar to + `dataclasses` or `pydantic` in that `.property` is used as a "field specifier" + to set options like the default value, and the type annotation is on the + class attribute. Documentation is in strings immediately following the + properties, which is understood by most automatic documentation tools. + + ``remaining`` is defined using a "getter" function, meaning this code will + be run each time ``counter.remaining`` is accessed. Its type will be the + return type of the function, and its docstring will come from the function + too. Setters with only a getter are read-only. + + Adding a "setter" to properties is optional, and makes them read-write. +""" + +from __future__ import annotations +import builtins +from typing import Annotated, Generic, Self, TypeAlias, TypeVar, overload, TYPE_CHECKING +import typing +from weakref import WeakSet + +from fastapi import Body, FastAPI +from pydantic import RootModel + +from labthings_fastapi.thing_description import type_to_dataschema +from labthings_fastapi.thing_description._model import ( + DataSchema, + Form, + PropertyAffordance, + PropertyOp, +) +from labthings_fastapi.utilities import labthings_data + +from .utilities.introspection import return_type +from .base_descriptor import BaseDescriptor +from .exceptions import ( + DocstringToMessage, + NotConnectedToServerError, + ReadOnlyPropertyError, +) + +if TYPE_CHECKING: + from .thing import Thing + + +# The following exceptions are raised only when creating/setting up properties. +class OverspecifiedDefaultError(DocstringToMessage, ValueError): + """The default value has been specified more than once. + + This error is raised when a `.DataProperty` is instantiated with both a + ``default`` value and a ``default_factory`` provided. + """ + + +class MissingDefaultError(DocstringToMessage, ValueError): + """The default value has not been specified. + + This error is raised when a `.DataProperty` is instantiated without a + ``default`` value or a ``default_factory`` function. + """ + + +class InconsistentTypeError(DocstringToMessage, TypeError): + """Different type hints have been given for a property. + + Every property should have a type hint, which may be provided in a few + different ways. If multiple type hints are provided, they must match. + See `.property` for more details. + """ + + +class MissingTypeError(DocstringToMessage, TypeError): + """No type hints have been given for a property. + + Every property should have a type hint, which may be provided in a few + different ways. This error indicates that no type hint was found. + """ + + +Value = TypeVar("Value") +ValueFactory: TypeAlias = callable[[None], Value] +ValueGetter: TypeAlias = callable[[Thing], Value] +ValueSetter: TypeAlias = callable[[Thing, Value], None] + + +# D103 ignores missing docstrings on overloads. This shouldn't be raised on overloads. +@overload # use as a decorator @property +def property(default: ValueGetter) -> FunctionalProperty[Value]: ... # noqa: D103 +@overload # use as `field: int = property(0)`` +def property(default: Value, *, readonly: bool = False) -> Value: ... # noqa: D103 +@overload # use as `field: int = property(default_factory=lambda: 0)` +def property(default_factory: ValueFactory, readonly: bool = False) -> Value: ... # noqa: D103 + + +def property( + default: Value | ValueGetter | None, + *, + default_factory: ValueFactory | None, + readonly: bool = False, +) -> Value | FunctionalProperty[Value]: + r"""Define a Property on a `.Thing`\ . + + This function may be used to define :ref:`wot_properties` in + two ways, as either a decorator or a field specifier. See the + examples in the :mod:`.thing_property` documentation. + + Properties should always have a type annotation. This type annotation + will be used in automatic documentation and also to serialise the value + to JSON when it is sent over the network. This mean that the type of your + property should either be JSON serialisable (i.e. simple built-in types) + or a subclass of `pydantic.BaseModel`. + + :param default: is the default value. Either this or + ``default_factory`` must be specified. + + When ``property`` is used as a decorator, the function + being decorated is passed as the first argument, which is + why this argument also accepts callable objects. Callable + default values are not supported. If you want to set your + default value with a function, see ``default_factory``. + :param default_factory: should return your default value. + This may be used as an alternative to ``default`` if you + need to use a mutable datatype. For example, it would be + better to specify ``default_factory=list`` than + ``default=[]`` because the second form would be shared + between all `.Thing`\ s with this property. + :param readonly: whether the property should be read-only + via the `.ThingClient` interface (i.e. over HTTP or via + a `.DirectThingClient`). This is automatically true if + ``property`` is used as a decorator and no setter is + specified. + + :return: a property descriptor, either a `.FunctionalProperty` + if used as a decorator, or a `.DataProperty` if used as + a field. + + **Typing Notes** + + This function has somewhat complicated type hints, for two reasons. + Firstly, it may be used either as a decorator or as a field specifier, + so ``default`` performs double duty as a default value or a getter. + Secondly, when used as a field specifier the type hint for the + property is attached to the attribute of the class to which the + function's output is assigned. This means ``property`` does not know + its type hint until after it's been called. + + When used as a field specifier, ``property`` returns a generic + `.DataProperty` descriptor instance, which will determine its type + when it is attached to the `.Thing`. The type hint on the return + value of ``property`` in that situation is a "white lie": we annotate + the return as having the same type as the ``default`` value (or the + ``default_factory`` return value). This means that type checkers such + as ``mypy`` will check that the default is valid for the type of the + field, and won't raise an error about assigning, for example, an + instance of ``DataProperty[int]`` to a field annotated as ``int``. + """ + if callable(default): + # If the default is callable, we're being used as a decorator + # without arguments. + func = default + return FunctionalProperty[return_type(func)]( + fget=func, + ) + return DataProperty( # type: ignore[return-type] + default=default, + default_factory=default_factory, + readonly=readonly, + ) + + +class BaseProperty(BaseDescriptor[Value], Generic[Value]): + """A descriptor that marks Properties on Things. + + This class is used to determine whether an attribute of a `.Thing` should + be treated as a Property (see :ref:`wot_properties` - essentially, it + means the value should be available over HTTP). + + `.BaseProperty` should not be used directly, instead it is recommended to + use `.property` to declare properties on you `.Thing` subclass. + """ + + +class DataProperty(BaseProperty[Value], Generic[Value]): + """A Property descriptor that acts like a regular variable. + + `.DataProperty` descriptors remember their value, and can be read and + written to like a regular Python variable. + """ + + def __init__( + self, + default: Value | None = None, + *, + default_factory: ValueFactory | None, + readonly: bool = False, + ): + """Create a property that acts like a regular variable. + + `.DataProperty` descriptors function just like variables, in that + they can be read and written to as attributes of the `.Thing` and + their value will be the same every time it is read (i.e. it changes + only when it is set). This differs from `.FunctionalProperty` which + uses a "getter" function just like `builtins.property` and may + return a different value each time. + + `.DataProperty` instances may always be set, when they are accessed + as an attribute of the `.Thing` instance. The ``readonly`` parameter + applies only to client code, whether it is remote or a + `.DirectThingClient` wrapper. + + The type of the property's value will be inferred either from the + type subscript or from an annotation on the class attribute. This + is done in ``__get_name__`` because neither is available during + ``__init__``. + + :param default: the default value. This or ``default_factory`` must + be provided. + :param default_factory: a function that returns the default value. + This is appropriate for datatypes such as lists, where using + a mutable default value can lead to odd behaviour. + :param readonly: if ``True``, the property may not be written to via + HTTP, or via `.DirectThingClient` objects, i.e. it may only be + set as an attribute of the `.Thing` and not from a client. + + :raises OverspecifiedDefaultError: if both a default and a default + factory function are specified. + :raises MissingDefaultError: if no default is provided. + """ + if default_factory is not None: + if default is not None: + raise OverspecifiedDefaultError() + self._default_value: Value = default_factory() + if default is None: + raise MissingDefaultError() + self._default_value: Value = default + self.readonly = readonly + self._type: type | None = None # Will be set in __set_name__ + + def __set_name__(self, owner: type[Thing], name: str) -> None: + """Take note of the name and type. + + This function is where we determine the type of the property. It may + be specified in two ways: either by subscripting ``DataProperty`` + or by annotating the attribute: + + .. code-block:: python + + class MyThing(Thing): + subscripted_property = DataProperty[int](0) + annotated_property: int = DataProperty(0) + + The second form often works better with autocompletion, though it is + preferred to use `.property` for consistent naming. + + Neither form allows us to access the type during ``__init__``, which + is why we find the type here. If there is a problem, exceptions raised + will appear to come from the class definition, so it's important to + include the name of the attribute. + + See :ref:`descriptors` for links to the Python docs about when + this function is called. + + :param owner: the `.Thing` subclass to which we are being attached. + :param name: the name to which we have been assigned. + + :raises InconsistentTypeError: if the type is specified twice and + the two types are not identical. + :raises MissingTypeError: if no type hints have been given. + """ + # Call BaseDescriptor so we remember the name + super().__set_name__(owner, name) + + # Check for type subscripts + if hasattr(self, "__orig_class__"): + # We have been instantiated with a subscript, e.g. BaseProperty[int]. + # + # __orig_class__ is set on generic classes when they are instantiated + # with a subscripted type. + self._type = typing.get_args(self.__orig_class__)[0] + + # Check for annotations on the parent class + annotations = typing.get_type_hints(owner, include_extras=True) + field_annotation = annotations.get(name, None) + if field_annotation is not None: + # We have been assigned to an annotated class attribute, e.g. + # myprop: int = BaseProperty(0) + if self._type is not None and self._type != field_annotation: + raise InconsistentTypeError( + f"Property {name} on {owner} has conflicting types.\n\n" + f"The field annotation of {field_annotation} conflicts " + f"with the inferred type of {self._type}." + ) + self._type = field_annotation + if self._type is None: + raise MissingTypeError( + f"No type hint was found for property {name} on {owner}." + ) + + def instance_get(self, obj: Thing) -> Value: + """Return the property's value. + + This will supply a default if the property has not yet been set. + + :param obj: The `.Thing` on which the property is being accessed. + :return: the value of the property. + """ + try: + return obj.__dict__[self.name] + except KeyError: + return self._default_value + + def __set__(self, obj: Thing, value: Value) -> None: + """Set the property's value. + + This sets the property's value, and notifies any observers. + + :param obj: the `.Thing` to which we are attached. + :param value: the new value for the property. + """ + obj.__dict__[self.name] = value + self.emit_changed_event(obj, value) + + def _observers_set(self, obj: Thing): + """Return the observers of this property. + + Each observer in this set will be notified when the property is changed. + See ``.ThingProperty.emit_changed_event`` + + :param obj: the `.Thing` to which we are attached. + + :return: the set of observers corresponding to ``obj``. + """ + ld = labthings_data(obj) + if self.name not in ld.property_observers: + ld.property_observers[self.name] = WeakSet() + return ld.property_observers[self.name] + + def emit_changed_event(self, obj: Thing, value: Value) -> None: + """Notify subscribers that the property has changed. + + This function is run when properties are updated. It must be run from + within a thread. This could be the `Invocation` thread of a running action, or + the property should be updated over via a client/http. It must be run from a + thread as it is communicating with the event loop via an `asyncio` blocking + portal and can cause deadlock if run in the event loop. + + :param obj: the `.Thing` to which we are attached. + :param value: the new property value, to be sent to observers. + + :raise NotConnectedToServerError: if the Thing that is calling the property + update is not connected to a server with a running event loop. + """ + runner = obj._labthings_blocking_portal + if not runner: + thing_name = obj.__class__.__name__ + msg = ( + f"Cannot emit property updated changed event. Is {thing_name} " + "connected to a running server?" + ) + raise NotConnectedToServerError(msg) + runner.start_task_soon( + self.emit_changed_event_async, + obj, + value, + ) + + async def emit_changed_event_async(self, obj: Thing, value: Value): + """Notify subscribers that the property has changed. + + This function may only be run in the `anyio` event loop. See + `.ThingProperty.emit_changed_event`. + + :param obj: the `.Thing` to which we are attached. + :param value: the new property value, to be sent to observers. + """ + for observer in self._observers_set(obj): + await observer.send( + {"messageType": "propertyStatus", "data": {self._name: value}} + ) + + def add_to_fastapi(self, app: FastAPI, thing: Thing) -> None: + """Add this action to a FastAPI app, bound to a particular Thing. + + :param app: The FastAPI application we are adding endpoints to. + :param thing: The `.Thing` we are adding the endpoints for. + """ + # We can't use the decorator in the usual way, because we'd need to + # annotate the type of `body` with `self.model` which is only defined + # at runtime. + # The solution below is to manually add the annotation, before passing + # the function to the decorator. + if not self.readonly: + + def set_property(body): # We'll annotate body later + if isinstance(body, RootModel): + body = body.root + return self.__set__(thing, body) + + set_property.__annotations__["body"] = Annotated[self.model, Body()] + app.put( + thing.path + self.name, + status_code=201, + response_description="Property set successfully", + summary=f"Set {self.title}", + description=f"## {self.title}\n\n{self.description or ''}", + )(set_property) + + @app.get( + thing.path + self.name, + response_model=self.model, + response_description=f"Value of {self.name}", + summary=self.title, + description=f"## {self.title}\n\n{self.description or ''}", + ) + def get_property(): + return self.__get__(thing) + + def property_affordance( + self, thing: Thing, path: str | None = None + ) -> PropertyAffordance: + """Represent the property in a Thing Description. + + :param thing: the `.Thing` to which we are attached. + :param path: the URL of the `.Thing`. If not present, we will retrieve + the ``path`` from ``thing``. + + :return: A description of the property in :ref:`wot_td` format. + """ + path = path or thing.path + ops = [PropertyOp.readproperty] + if not self.readonly: + ops.append(PropertyOp.writeproperty) + forms = [ + Form[PropertyOp]( + href=path + self.name, + op=ops, + ), + ] + data_schema: DataSchema = type_to_dataschema(self.model) + pa: PropertyAffordance = PropertyAffordance( + title=self.title, + forms=forms, + description=self.description, + ) + # We merge the data schema with the property affordance (which subclasses the + # DataSchema model) with the affordance second so its values take priority. + # Note that this works because all of the fields that get filled in by + # DataSchema are optional - so the PropertyAffordance is still valid without + # them. + return PropertyAffordance( + **{ + **data_schema.model_dump(exclude_none=True), + **pa.model_dump(exclude_none=True), + } + ) + + +def setting( + default: Value | None = None, + *, + default_factory: ValueFactory | None = None, + readonly: bool = False, +) -> ThingSetting[Value]: + r"""Define a Setting on a `.Thing`\ . + + This function defines a setting, which is a special Property that will + be saved to disk and reloaded next time the `.Thing` is run. It is + otherwise very similar to `.property` with the exception that it may + only be used as a field, i.e. not as a decorator. Settings may not + be implemented with getter and setter methods, as this can conflict + with loading and saving to disk. + + :param default: is the default value. Either this or + ``default_factory`` must be specified. + :param default_factory: should return your default value. + This may be used as an alternative to ``default`` if you + need to use a mutable datatype. For example, it would be + better to specify ``default_factory=list`` than + ``default=[]`` because the second form would be shared + between all `.Thing`\ s with this setting. + :param readonly: whether the setting should be read-only + via the `.ThingClient` interface (i.e. over HTTP or via + a `.DirectThingClient`). + + :return: a setting descriptor. + + **Typing Notes** + + The return type of this function is a "white lie" in order to allow + dataclass-style type annotations + """ + return ThingSetting( + default=default, + default_factory=default_factory, + readonly=readonly, + ) + + +class ThingSetting(DataProperty[Value], Generic[Value]): + """A `.ThingProperty` that persists on disk. + + A setting can be accessed via the HTTP API and is persistent between sessions. + + A `.ThingSetting` is a `.ThingProperty` with extra functionality for triggering + a `.Thing` to save its settings. + + Note: If a setting is mutated rather than assigned to, this will not trigger saving. + For example: if a Thing has a setting called `dictsetting` holding the dictionary + `{"a": 1, "b": 2}` then `self.dictsetting = {"a": 2, "b": 2}` would trigger saving + but `self.dictsetting[a] = 2` would not, as the setter for `dictsetting` is never + called. + + The setting otherwise acts just like a normal variable. + """ + + def __set__(self, obj: Thing, value: Value): + """Set the setting's value. + + This will cause the settings to be saved to disk. + + :param obj: the `.Thing` to which we are attached. + :param value: the new value of the setting. + """ + super().__set__(obj, value) + obj.save_settings() + + def set_without_emit(self, obj: Thing, value: Value): + """Set the property's value, but do not emit event to notify the server. + + This function is not expected to be used externally. It is called during + initial setup so that the setting can be set from disk before the server + is fully started. + + :param obj: the `.Thing` to which we are attached. + :param value: the new value of the setting. + """ + obj.__dict__[self.name] = value + if self._setter: + self._setter(obj, value) + + +class FunctionalProperty(BaseProperty[Value], Generic[Value], builtins.property): + """A property that uses a getter and a setter. + + For properties that should work like variables, use `.DataProperty`. For + properties that need to run code every time they are read, use this class. + + Functional properties should work very much like Python's `builtins.property` + except that they are also available over HTTP. + """ + + def __init__( + self, + fget: ValueGetter, + ): + """Set up a FunctionalProperty. + + Create a descriptor for a property that uses a getter function. + + This class also inherits from `builtins.property` to help type checking + tools understand that it functions like a property. + + :param fget: the getter function, called when the property is read. + """ + self._fget: ValueGetter = fget + self._fset: ValueSetter | None = None + self.readonly: bool = True + + # Note: DOC201 and DOC401 are ignored on these properties, as + # they only apply to functions. Pydoclint doesn't recognise these + # as properties, because we use `builtins.property` and not `property`. + @builtins.property + def fget(self) -> ValueGetter: # noqa: DOC201 + """The getter function.""" # noqa: D401 + return self._fget + + @builtins.property + def fset(self) -> ValueSetter | None: # noqa: DOC201 + """The setter function.""" # noqa: D401 + return self._fset + + @builtins.property + def fdel(self) -> None: # noqa: DOC201 + """The deleter function. + + This function always returns ``None`` as deleters are not yet supported. + """ # noqa: D401 + return None + + def getter(self, fget: ValueGetter) -> Self: + """Set the getter function of the property. + + This function returns the descriptor, so it may be used as a decorator. + If the function has a docstring, it will be used as the property docstring. + + :param fget: The new getter function. + :return: this descriptor (i.e. ``self``). This allows use as a decorator. + """ + self._fget = fget + if fget.__doc__: + self.__doc__ = fget.__doc__ + + def setter(self, fset: ValueSetter) -> Self: + """Set the setter function of the property. + + This function returns the descriptor, so it may be used as a decorator. + + Once a setter has been added to a property, it will automatically become + writeable from client code (over HTTP and via `.DirectThingClient`). + To override this behaviour you may set ``readonly`` back to ``True``. + + .. code-block:: python + + class MyThing(lt.Thing): + def __init__(self): + self._myprop: int = 0 + + @lt.property + def myprop(self) -> int: + "An example property that is an integer" + return self._myprop + + @myprop.setter + def myprop(self, val: int): + self._myprop = val + + myprop.readonly = True # Prevent client code from setting it + + :param fset: The new setter function. + :return: this descriptor (i.e. ``self``). This allows use as a decorator. + """ + self._fset = fset + self.readonly = False + + def deleter(self, fdel: callable) -> Self: + """Set a deleter function. Currently unsupported. + + :param fdel: The function called when the attribute is deleted. + :return: The descriptor (i.e. ``self``). + + :raises NotImplementedError: every time, because it is not supported. + """ + raise NotImplementedError( + "Deleter functions are not supported for FunctionalProperty." + ) + + def instance_get(self, obj: Thing) -> Value: + """Get the value of the property. + + :param obj: the `.Thing` on which the attribute is accessed. + :return: the value of the property. + """ + return self.fget(obj) + + def __set__(self, obj: Thing, value: Value): + """Set the value of the property. + + :param obj: the `.Thing` on which the attribute is accessed. + :param value: the value of the property. + + :raises ReadOnlyPropertyError: if the property cannot be set. + """ + if self.fset: + self.fset(obj, value) + raise ReadOnlyPropertyError(f"Property {self.name} of {obj} has no setter.") From da345fb591db9bbd48c3d1f46ab55ce458b634f0 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Thu, 24 Jul 2025 23:00:22 +0100 Subject: [PATCH 03/59] Update the rest of the library to use the new names This includes a few typing fixes, but does not yet pass mypy --- src/labthings_fastapi/__init__.py | 8 +- src/labthings_fastapi/base_descriptor.py | 7 +- src/labthings_fastapi/client/in_server.py | 8 +- src/labthings_fastapi/decorators/__init__.py | 76 ------------------- src/labthings_fastapi/descriptors/__init__.py | 4 - src/labthings_fastapi/exceptions.py | 13 ++-- src/labthings_fastapi/thing.py | 14 ++-- src/labthings_fastapi/thing_property.py | 55 ++++++++++---- src/labthings_fastapi/utilities/__init__.py | 2 +- 9 files changed, 70 insertions(+), 117 deletions(-) diff --git a/src/labthings_fastapi/__init__.py b/src/labthings_fastapi/__init__.py index 49128591..e966d170 100644 --- a/src/labthings_fastapi/__init__.py +++ b/src/labthings_fastapi/__init__.py @@ -20,10 +20,8 @@ """ from .thing import Thing -from .thing_property import property, ThingSetting +from .thing_property import property, setting, DataProperty, ThingSetting from .decorators import ( - thing_property, - thing_setting, thing_action, fastapi_endpoint, ) @@ -43,9 +41,9 @@ __all__ = [ "Thing", "property", + "setting", + "DataProperty", "ThingSetting", - "thing_property", - "thing_setting", "thing_action", "fastapi_endpoint", "deps", diff --git a/src/labthings_fastapi/base_descriptor.py b/src/labthings_fastapi/base_descriptor.py index 815f186c..a20d6434 100644 --- a/src/labthings_fastapi/base_descriptor.py +++ b/src/labthings_fastapi/base_descriptor.py @@ -5,6 +5,7 @@ the code that implements it. """ +from __future__ import annotations from typing import overload, Generic, Self, TypeVar, TYPE_CHECKING from .utilities.introspection import get_summary @@ -35,7 +36,7 @@ class BaseDescriptor(Generic[Value]): e.g. `property`. """ - def __init__(self): + def __init__(self) -> None: """Initialise a BaseDescriptor.""" self._name: str | None = None self._title: str | None = None @@ -125,7 +126,7 @@ def description(self) -> str | None: def __get__(self, obj: Thing, type: type | None = None) -> Value: ... # noqa: D105 @overload - def __get__(self, type: type) -> Self: ... # noqa: D105 + def __get__(self, obj: None, type: type) -> Self: ... # noqa: D105 def __get__(self, obj: Thing | None, type: type | None = None) -> Value | Self: """Return the value or the descriptor, as per `property`. @@ -144,7 +145,7 @@ def __get__(self, obj: Thing | None, type: type | None = None) -> Value | Self: accessed on an instance, or the descriptor object if accessed on a class. """ if obj is not None: - return self.instance_get(obj, type) + return self.instance_get(obj) return self def instance_get(self, obj: Thing) -> Value: diff --git a/src/labthings_fastapi/client/in_server.py b/src/labthings_fastapi/client/in_server.py index 17b99327..f3880d12 100644 --- a/src/labthings_fastapi/client/in_server.py +++ b/src/labthings_fastapi/client/in_server.py @@ -21,7 +21,7 @@ from pydantic import BaseModel from ..descriptors.action import ActionDescriptor -from ..descriptors.property import ThingProperty +from ..thing_property import BaseProperty from ..utilities import attributes from . import PropertyClientDescriptor from ..thing import Thing @@ -219,7 +219,7 @@ def action_method(self, **kwargs): def add_property( - attrs: dict[str, Any], property_name: str, property: ThingProperty + attrs: dict[str, Any], property_name: str, property: BaseProperty ) -> None: """Add a property to a DirectThingClient subclass. @@ -238,7 +238,7 @@ def add_property( property.model, description=property.description, writeable=not property.readonly, - readable=True, # TODO: make this configurable in ThingProperty + readable=True, ) @@ -283,7 +283,7 @@ def init_proxy(self, request: Request, **dependencies: Mapping[str, Any]): } dependencies: list[inspect.Parameter] = [] for name, item in attributes(thing_class): - if isinstance(item, ThingProperty): + if isinstance(item, BaseProperty): add_property(client_attrs, name, item) elif isinstance(item, ActionDescriptor): if actions is None or name in actions: diff --git a/src/labthings_fastapi/decorators/__init__.py b/src/labthings_fastapi/decorators/__init__.py index 0cef9b10..b1950b76 100644 --- a/src/labthings_fastapi/decorators/__init__.py +++ b/src/labthings_fastapi/decorators/__init__.py @@ -40,12 +40,9 @@ from typing import Optional, Callable, overload from ..descriptors import ( ActionDescriptor, - ThingProperty, - ThingSetting, EndpointDescriptor, HTTPMethod, ) -from ..utilities.introspection import return_type def mark_thing_action(func: Callable, **kwargs) -> ActionDescriptor: @@ -123,79 +120,6 @@ def thing_action( return partial(mark_thing_action, **kwargs) -def thing_property(func: Callable) -> ThingProperty: - """Mark a method of a Thing as a LabThings Property. - - This should be used as a decorator with a getter and a setter - just like a standard python `property` decorator. If extra functionality - is not required in the decorator, then using the `.ThingProperty` class - directly may allow for clearer code - - Properties should always have a type annotation. This type annotation - will be used in automatic documentation and also to serialise the value - to JSON when it is sent over th network. This mean that the type of your - property should either be JSON serialisable (i.e. simple built-in types) - or a subclass of `pydantic.BaseModel`. - - :param func: A method to use as the getter for the new property. - - :return: A `.ThingProperty` descriptor that works like `property` but - allows the value to be read over HTTP. - """ - # Replace the function with a `Descriptor` that's a `ThingProperty` - return ThingProperty( - return_type(func), - readonly=True, - observable=False, - getter=func, - ) - - -def thing_setting(func: Callable) -> ThingSetting: - """Mark a method of a Thing as a LabThings Setting. - - A setting is a property that is saved to disk, so it persists even when - the LabThings server is restarted. - - This should be used as a decorator with a getter and a setter - just like a standard python property decorator. If extra functionality - is not required in the decorator, then using the `ThingSetting` class - directly may allow for clearer code where the property works like a - variable. - - When creating a setting using this decorator, you must always add a setter - as it is used to load the value from disk. This follows the same syntax as - for `property`, i.e. a second function with the same name, decorated with - ``@my_property_name.setter``. - - A type annotation is required, and should follow the same constraints as - for :deco:`thing_property`. - - If the type is a pydantic BaseModel, then the setter must also be able to accept - the dictionary representation of this BaseModel as this is what will be used to - set the Setting when loading from disk on starting the server. - - .. note:: - If a setting is mutated rather than set, this will not trigger saving. - For example: if a Thing has a setting called ``dictsetting`` holding the - dictionary ``{"a": 1, "b": 2}`` then ``self.dictsetting = {"a": 2, "b": 2}`` - would trigger saving but ``self.dictsetting[a] = 2`` would not, as the - setter for ``dictsetting`` is never called. - - :param func: A method to use as the getter for the new property. - - :return: A `.ThingSetting` descriptor that works like `property` but - allows the value to be read over HTTP and saves it to disk. - """ - # Replace the function with a `Descriptor` that's a `ThingSetting` - return ThingSetting( - return_type(func), - readonly=True, - observable=False, - getter=func, - ) - - def fastapi_endpoint( method: HTTPMethod, path: Optional[str] = None, **kwargs ) -> Callable[[Callable], EndpointDescriptor]: diff --git a/src/labthings_fastapi/descriptors/__init__.py b/src/labthings_fastapi/descriptors/__init__.py index 404cce21..581ea7ad 100644 --- a/src/labthings_fastapi/descriptors/__init__.py +++ b/src/labthings_fastapi/descriptors/__init__.py @@ -4,15 +4,11 @@ """ from .action import ActionDescriptor -from .property import ThingProperty -from .property import ThingSetting from .endpoint import EndpointDescriptor from .endpoint import HTTPMethod __all__ = [ "ActionDescriptor", - "ThingProperty", - "ThingSetting", "EndpointDescriptor", "HTTPMethod", ] diff --git a/src/labthings_fastapi/exceptions.py b/src/labthings_fastapi/exceptions.py index 786fe14a..7eb2c92e 100644 --- a/src/labthings_fastapi/exceptions.py +++ b/src/labthings_fastapi/exceptions.py @@ -21,14 +21,17 @@ def __init__(self, message: str | None): :param message: the optional message. """ + # type: ignore[call-arg] is used here because mypy can't know + # that this is a mixin, and super() will be an exception (which does + # accept a string argument to `__init__`). doc = inspect.cleandoc(self.__doc__) if self.__doc__ else None if message: if doc and self.append_to_message: - super().__init__(message + "\n\n" + doc) + super().__init__(message + "\n\n" + doc) # type: ignore[call-arg] else: - super().__init__(message) + super().__init__(message) # type: ignore[call-arg] elif doc: - super().__init__(doc) + super().__init__(doc) # type: ignore[call-arg] else: super().__init__() @@ -36,8 +39,8 @@ def __init__(self, message: str | None): class NotConnectedToServerError(DocstringToMessage, RuntimeError): """The Thing is not connected to a server. - This exception is called if a ThingAction is called or - is a ThingProperty is updated on a Thing that is not + This exception is called if an Action is called or + a `.DataProperty` is updated on a Thing that is not connected to a ThingServer. A server connection is needed to manage asynchronous behaviour. """ diff --git a/src/labthings_fastapi/thing.py b/src/labthings_fastapi/thing.py index 4ef50820..612c0ddc 100644 --- a/src/labthings_fastapi/thing.py +++ b/src/labthings_fastapi/thing.py @@ -21,7 +21,8 @@ from pydantic import BaseModel -from .descriptors import ThingProperty, ThingSetting, ActionDescriptor +from .thing_property import BaseProperty, ThingSetting +from .descriptors import ActionDescriptor from .thing_description._model import ThingDescription, NoSecurityScheme from .utilities import class_attributes from .thing_description import validation @@ -57,9 +58,12 @@ class Thing: ``finally:`` block. * Properties and Actions are defined using decorators: the :deco:`.thing_action` decorator declares a method to be an action, which will run when it's triggered, - and the :deco:`.thing_property` decorator (or `.ThingProperty` descriptor) does - the same for a property. See the documentation on those functions for more - detail. + and the :deco:`.property` decorator does the same for a property. + + Properties may also be defined using dataclass-style syntax, if they do + not need getter and setter functions. + + See the documentation on those functions for more detail. * `title` will be used in various places as the human-readable name of your Thing, so it makes sense to set this in a subclass. @@ -344,7 +348,7 @@ def observe_property(self, property_name: str, stream: ObjectSendStream) -> None :raise KeyError: if the requested name is not defined on this Thing. """ prop = getattr(self.__class__, property_name) - if not isinstance(prop, ThingProperty): + if not isinstance(prop, BaseProperty): raise KeyError(f"{property_name} is not a LabThings Property") prop._observers_set(self).add(stream) diff --git a/src/labthings_fastapi/thing_property.py b/src/labthings_fastapi/thing_property.py index b1d3a1bf..46a7c724 100644 --- a/src/labthings_fastapi/thing_property.py +++ b/src/labthings_fastapi/thing_property.py @@ -46,7 +46,15 @@ class attribute. Documentation is in strings immediately following the from __future__ import annotations import builtins -from typing import Annotated, Generic, Self, TypeAlias, TypeVar, overload, TYPE_CHECKING +from typing import ( + Annotated, + Callable, + Generic, + Self, + TypeVar, + overload, + TYPE_CHECKING, +) import typing from weakref import WeakSet @@ -108,10 +116,13 @@ class MissingTypeError(DocstringToMessage, TypeError): """ +# fmt: off Value = TypeVar("Value") -ValueFactory: TypeAlias = callable[[None], Value] -ValueGetter: TypeAlias = callable[[Thing], Value] -ValueSetter: TypeAlias = callable[[Thing, Value], None] +if TYPE_CHECKING: + ValueFactory = Callable[[None,], Value] + ValueGetter = Callable[[Thing,], Value] + ValueSetter = Callable[[Thing, Value], None] +# fmt: on # D103 ignores missing docstrings on overloads. This shouldn't be raised on overloads. @@ -124,9 +135,9 @@ def property(default_factory: ValueFactory, readonly: bool = False) -> Value: .. def property( - default: Value | ValueGetter | None, + default: Value | ValueGetter | None = None, *, - default_factory: ValueFactory | None, + default_factory: ValueFactory | None = None, readonly: bool = False, ) -> Value | FunctionalProperty[Value]: r"""Define a Property on a `.Thing`\ . @@ -192,7 +203,7 @@ def property( return FunctionalProperty[return_type(func)]( fget=func, ) - return DataProperty( # type: ignore[return-type] + return DataProperty( # type: ignore[return-value] default=default, default_factory=default_factory, readonly=readonly, @@ -355,7 +366,7 @@ def _observers_set(self, obj: Thing): """Return the observers of this property. Each observer in this set will be notified when the property is changed. - See ``.ThingProperty.emit_changed_event`` + See ``.DataProperty.emit_changed_event`` :param obj: the `.Thing` to which we are attached. @@ -399,7 +410,7 @@ async def emit_changed_event_async(self, obj: Thing, value: Value): """Notify subscribers that the property has changed. This function may only be run in the `anyio` event loop. See - `.ThingProperty.emit_changed_event`. + `.DataProperty.emit_changed_event`. :param obj: the `.Thing` to which we are attached. :param value: the new property value, to be sent to observers. @@ -494,13 +505,29 @@ def setting( ) -> ThingSetting[Value]: r"""Define a Setting on a `.Thing`\ . + A setting is a property that is saved to disk + This function defines a setting, which is a special Property that will - be saved to disk and reloaded next time the `.Thing` is run. It is - otherwise very similar to `.property` with the exception that it may - only be used as a field, i.e. not as a decorator. Settings may not + be saved to disk, so it persists even when the LabThings server is + restarted. It is otherwise very similar to `.property` with the exception + that it may only be used as a field, i.e. not as a decorator. Settings may not be implemented with getter and setter methods, as this can conflict with loading and saving to disk. + A type annotation is required, and should follow the same constraints as + for :deco:`.property`. + + If the type is a pydantic BaseModel, then the setter must also be able to accept + the dictionary representation of this BaseModel as this is what will be used to + set the Setting when loading from disk on starting the server. + + .. note:: + If a setting is mutated rather than set, this will not trigger saving. + For example: if a Thing has a setting called ``dictsetting`` holding the + dictionary ``{"a": 1, "b": 2}`` then ``self.dictsetting = {"a": 2, "b": 2}`` + would trigger saving but ``self.dictsetting[a] = 2`` would not, as the + setter for ``dictsetting`` is never called. + :param default: is the default value. Either this or ``default_factory`` must be specified. :param default_factory: should return your default value. @@ -528,11 +555,11 @@ def setting( class ThingSetting(DataProperty[Value], Generic[Value]): - """A `.ThingProperty` that persists on disk. + """A `.DataProperty` that persists on disk. A setting can be accessed via the HTTP API and is persistent between sessions. - A `.ThingSetting` is a `.ThingProperty` with extra functionality for triggering + A `.ThingSetting` is a `.DataProperty` with extra functionality for triggering a `.Thing` to save its settings. Note: If a setting is mutated rather than assigned to, this will not trigger saving. diff --git a/src/labthings_fastapi/utilities/__init__.py b/src/labthings_fastapi/utilities/__init__.py index 4b5008a4..ca7b8885 100644 --- a/src/labthings_fastapi/utilities/__init__.py +++ b/src/labthings_fastapi/utilities/__init__.py @@ -57,7 +57,7 @@ class LabThingsObjectData: property_observers: Dict[str, WeakSet] = Field(default_factory=dict) r"""The observers added to each property. - Keys are property names, values are weak sets used by `.ThingProperty`\ . + Keys are property names, values are weak sets used by `.DataProperty`\ . """ action_observers: Dict[str, WeakSet] = Field(default_factory=dict) r"""The observers added to each action. From 54b19f0005d07c6959e8bd150bd9656227b54839 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Thu, 24 Jul 2025 23:17:11 +0100 Subject: [PATCH 04/59] Update examples and test suite Tests now all collect, though several are still failing and the websocket test is hanging. The DocstringToMessage mixin was not working, this is now fixed. I now use ... as a default value for `default` in properties/settings, because None is a legitimate value. --- docs/source/quickstart/counter.py | 5 ++- .../example_things/__init__.py | 18 ++++------- src/labthings_fastapi/exceptions.py | 2 +- src/labthings_fastapi/thing_property.py | 32 +++++++++++++------ tests/test_action_cancel.py | 17 ++++------ tests/test_action_manager.py | 5 ++- tests/test_dependency_metadata.py | 2 +- tests/test_properties.py | 21 ++++++------ tests/test_settings.py | 20 +++++------- tests/test_thing_lifecycle.py | 3 +- 10 files changed, 64 insertions(+), 61 deletions(-) diff --git a/docs/source/quickstart/counter.py b/docs/source/quickstart/counter.py index ea2990c5..89a455c3 100644 --- a/docs/source/quickstart/counter.py +++ b/docs/source/quickstart/counter.py @@ -22,9 +22,8 @@ def slowly_increase_counter(self) -> None: time.sleep(1) self.increment_counter() - counter = lt.ThingProperty( - model=int, initial_value=0, readonly=True, description="A pointless counter" - ) + counter: int = lt.property(0, readonly=True) + "A pointless counter" if __name__ == "__main__": diff --git a/src/labthings_fastapi/example_things/__init__.py b/src/labthings_fastapi/example_things/__init__.py index 3c8b045f..3341a2d2 100644 --- a/src/labthings_fastapi/example_things/__init__.py +++ b/src/labthings_fastapi/example_things/__init__.py @@ -7,8 +7,8 @@ import time from typing import Any, Optional, Annotated from labthings_fastapi.thing import Thing -from labthings_fastapi.decorators import thing_action, thing_property -from labthings_fastapi.descriptors import ThingProperty +from labthings_fastapi.decorators import thing_action +from labthings_fastapi.thing_property import property as lt_property from pydantic import Field @@ -94,15 +94,11 @@ def slowly_increase_counter(self, increments: int = 60, delay: float = 1): time.sleep(delay) self.increment_counter() - counter = ThingProperty( - model=int, initial_value=0, readonly=True, description="A pointless counter" - ) + counter: int = lt_property(0, readonly=True) + "A pointless counter" - foo = ThingProperty( - model=str, - initial_value="Example", - description="A pointless string for demo purposes.", - ) + foo: str = lt_property("Example") + "A pointless string for demo purposes." @thing_action def action_without_arguments(self) -> None: @@ -129,7 +125,7 @@ def broken_action(self): """ raise RuntimeError("This is a broken action") - @thing_property + @lt_property def broken_property(self): """Raise an exception when the property is accessed. diff --git a/src/labthings_fastapi/exceptions.py b/src/labthings_fastapi/exceptions.py index 7eb2c92e..e0ecb724 100644 --- a/src/labthings_fastapi/exceptions.py +++ b/src/labthings_fastapi/exceptions.py @@ -16,7 +16,7 @@ class DocstringToMessage: append_to_message: bool = True - def __init__(self, message: str | None): + def __init__(self, message: str | None = None): """Initialise an error with a message or its docstring. :param message: the optional message. diff --git a/src/labthings_fastapi/thing_property.py b/src/labthings_fastapi/thing_property.py index 46a7c724..3f149bd5 100644 --- a/src/labthings_fastapi/thing_property.py +++ b/src/labthings_fastapi/thing_property.py @@ -46,6 +46,7 @@ class attribute. Documentation is in strings immediately following the from __future__ import annotations import builtins +from types import EllipsisType from typing import ( Annotated, Callable, @@ -135,7 +136,7 @@ def property(default_factory: ValueFactory, readonly: bool = False) -> Value: .. def property( - default: Value | ValueGetter | None = None, + default: Value | ValueGetter | EllipsisType = ..., *, default_factory: ValueFactory | None = None, readonly: bool = False, @@ -195,6 +196,11 @@ def property( as ``mypy`` will check that the default is valid for the type of the field, and won't raise an error about assigning, for example, an instance of ``DataProperty[int]`` to a field annotated as ``int``. + + Finally, the type of the ``default`` argument includes `.EllipsisType` + so that we can use ``...`` as its default value. This allows us to + distinguish between ``default`` not being set (``...``) and a desired + default value of ``None``. """ if callable(default): # If the default is callable, we're being used as a decorator @@ -231,7 +237,7 @@ class DataProperty(BaseProperty[Value], Generic[Value]): def __init__( self, - default: Value | None = None, + default: Value | EllipsisType = ..., *, default_factory: ValueFactory | None, readonly: bool = False, @@ -256,7 +262,9 @@ def __init__( ``__init__``. :param default: the default value. This or ``default_factory`` must - be provided. + be provided. Note that, as ``None`` is a valid default value, + this uses ``...`` instead as a way of checking whether ``default`` + has been set. :param default_factory: a function that returns the default value. This is appropriate for datatypes such as lists, where using a mutable default value can lead to odd behaviour. @@ -268,13 +276,17 @@ def __init__( factory function are specified. :raises MissingDefaultError: if no default is provided. """ - if default_factory is not None: - if default is not None: - raise OverspecifiedDefaultError() - self._default_value: Value = default_factory() - if default is None: + if default_factory is not None and default is not ...: + raise OverspecifiedDefaultError() + if default_factory is None and default is ...: raise MissingDefaultError() - self._default_value: Value = default + # The default value will come from whichever of `default` and `default_factory` + # is specified. The two checks above ensure that exactly one will exist. + self._default_value: Value + if default_factory: + self._default_value = default_factory + else: + self._default_value = default self.readonly = readonly self._type: type | None = None # Will be set in __set_name__ @@ -498,7 +510,7 @@ def property_affordance( def setting( - default: Value | None = None, + default: Value | EllipsisType = ..., *, default_factory: ValueFactory | None = None, readonly: bool = False, diff --git a/tests/test_action_cancel.py b/tests/test_action_cancel.py index a5a4ad49..03ee939f 100644 --- a/tests/test_action_cancel.py +++ b/tests/test_action_cancel.py @@ -10,16 +10,13 @@ class CancellableCountingThing(lt.Thing): - counter = lt.ThingProperty(int, 0, observable=False) - check = lt.ThingProperty( - bool, - False, - observable=False, - description=( - "This variable is used to check that the action can detect a cancel event " - "and react by performing another task, in this case, setting this variable." - ), - ) + counter: int = lt.property(0) + check: bool = lt.property(False) + """Whether the count has been cancelled. + + This variable is used to check that the action can detect a cancel event + and react by performing another task, in this case, setting this variable. + """ @lt.thing_action def count_slowly(self, cancel: lt.deps.CancelHook, n: int = 10): diff --git a/tests/test_action_manager.py b/tests/test_action_manager.py index 4ae2ab4f..606d1be4 100644 --- a/tests/test_action_manager.py +++ b/tests/test_action_manager.py @@ -13,9 +13,8 @@ def increment_counter(self): """Increment the counter""" self.counter += 1 - counter = lt.ThingProperty( - model=int, initial_value=0, readonly=True, description="A pointless counter" - ) + counter: int = lt.property(0, readonly=True) + "A pointless counter" thing = TestThing() diff --git a/tests/test_dependency_metadata.py b/tests/test_dependency_metadata.py index 2536d8f2..d6449438 100644 --- a/tests/test_dependency_metadata.py +++ b/tests/test_dependency_metadata.py @@ -13,7 +13,7 @@ def __init__(self): lt.Thing.__init__(self) self._a = 0 - @lt.thing_property + @lt.property def a(self): return self._a diff --git a/tests/test_properties.py b/tests/test_properties.py index 3946c7e1..05dd0f50 100644 --- a/tests/test_properties.py +++ b/tests/test_properties.py @@ -9,18 +9,21 @@ class TestThing(lt.Thing): - boolprop = lt.ThingProperty(bool, False, description="A boolean property") - stringprop = lt.ThingProperty(str, "foo", description="A string property") + boolprop: bool = lt.property(False) + "A boolean property" + + stringprop: str = lt.property("foo") + "A string property" _undoc = None - @lt.thing_property + @lt.property def undoc(self): return self._undoc _float = 1.0 - @lt.thing_property + @lt.property def floatprop(self) -> float: return self._float @@ -45,12 +48,12 @@ def toggle_boolprop_from_thread(self): def test_instantiation_with_type(): """ - Check the internal model (data type) of the ThingSetting descriptor is a BaseModel + Check the internal model (data type) of the DataProperty descriptor is a BaseModel To send the data over HTTP LabThings-FastAPI uses Pydantic models to describe data types. """ - prop = lt.ThingProperty(bool, False) + prop: bool = lt.property(False) assert issubclass(prop.model, BaseModel) @@ -59,7 +62,7 @@ class MyModel(BaseModel): a: int = 1 b: float = 2.0 - prop = lt.ThingProperty(MyModel, MyModel()) + prop: MyModel = lt.property(MyModel()) assert prop.model is MyModel @@ -71,7 +74,7 @@ def test_property_get_and_set(): assert after_value.json() == test_str -def test_ThingProperty(): +def test_boolprop(): with TestClient(server.app) as client: r = client.get("/thing/boolprop") assert r.json() is False @@ -123,7 +126,7 @@ def test_setting_from_thread(): def test_setting_without_event_loop(): - """Test that an exception is raised if updating a ThingProperty + """Test that an exception is raised if updating a DataProperty without connecting the Thing to a running server with an event loop. """ # This test may need to change, if we change the intended behaviour diff --git a/tests/test_settings.py b/tests/test_settings.py index 50bb7656..3b8638fb 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -11,21 +11,17 @@ class TestThing(lt.Thing): - boolsetting = lt.ThingSetting(bool, False, description="A boolean setting") - stringsetting = lt.ThingSetting(str, "foo", description="A string setting") - dictsetting = lt.ThingSetting( - dict, {"a": 1, "b": 2}, description="A dictionary setting" - ) + boolsetting: bool = lt.setting(False) + "A boolean setting" - _float = 1.0 + stringsetting: str = lt.setting("foo") + "A string setting" - @lt.thing_setting - def floatsetting(self) -> float: - return self._float + dictsetting: dict = lt.setting(default_factory=lambda: {"a": 1, "b": 2}) + "A dictionary setting" - @floatsetting.setter - def floatsetting(self, value: float): - self._float = value + floatsetting: float = lt.setting(1.0) + "A float setting" @lt.thing_action def toggle_boolsetting(self): diff --git a/tests/test_thing_lifecycle.py b/tests/test_thing_lifecycle.py index 2d7331b9..86062bce 100644 --- a/tests/test_thing_lifecycle.py +++ b/tests/test_thing_lifecycle.py @@ -3,7 +3,8 @@ class TestThing(lt.Thing): - alive = lt.ThingProperty(bool, False, description="Is the thing alive?") + alive: bool = lt.property(False) + "Whether the thing is alive." def __enter__(self): print("setting up TestThing from __enter__") From 7ad075555933671a7de7686bfefb619d3489a264 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Fri, 25 Jul 2025 23:22:09 +0100 Subject: [PATCH 05/59] Fix test_property tests This caught a few errors in my rearrangement - most notably that only DataProperties were getting added to the HTTP API. I've fixed things so all properties now appear on HTTP, and improved model generation so it has helpful errors and works for both data and functional properties. I also removed the numpy mypy plugin because the warning got annoying. --- pyproject.toml | 2 +- src/labthings_fastapi/base_descriptor.py | 52 ++++- src/labthings_fastapi/exceptions.py | 2 +- src/labthings_fastapi/thing.py | 4 +- src/labthings_fastapi/thing_property.py | 241 +++++++++++++---------- tests/test_properties.py | 23 ++- 6 files changed, 206 insertions(+), 118 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index d6c0e96d..fbec4856 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -93,7 +93,7 @@ preview = true "docs/*" = ["D", "DOC"] [tool.mypy] -plugins = ["pydantic.mypy", "numpy.typing.mypy_plugin"] +plugins = ["pydantic.mypy"] [tool.flake8] extend-ignore = [ diff --git a/src/labthings_fastapi/base_descriptor.py b/src/labthings_fastapi/base_descriptor.py index a20d6434..2192407e 100644 --- a/src/labthings_fastapi/base_descriptor.py +++ b/src/labthings_fastapi/base_descriptor.py @@ -17,11 +17,31 @@ """The value returned by the descriptor, when called on an instance.""" -class NameNotSetError(AttributeError): - """Descriptor name is not yet available. +class DescriptorNotAddedToClassError(RuntimeError): + """Descriptor has not yet been added to a class. - This error is raised if the name of an affordance is accessed - before ``__set_name__`` has been called on the descriptor. + This error is raised if certain properties of descriptors are accessed + before ``__set_name__`` has been called on the descriptor. ``__set_name`` + is part of the descriptor protocol, and is called when a class is defined + to notify the descriptor of its name and owning class. + + If you see this error, it often means that a descriptor has been instantiated + but not attached to a class, for example: + + .. code-block:: python + + import labthings as lt + + + class Test(lt.Thing): + myprop: int = lt.property(0) # This is OK + + + orphaned_prop: int = lt.property(0) # Not OK + + Test.myprop.model # Evaluates to a pydantic model + + orphaned_prop.model # Raises this exception """ @@ -44,6 +64,9 @@ def __init__(self) -> None: # We set the instance __doc__ to None so the descriptor class docstring # doesn't get picked up by OpenAPI/Thing Description. self.__doc__ = None + # We explicitly check when __set_name__ is called, so we can raise helpful + # errors + self._set_name_called: bool = False def __set_name__(self, owner: type[Thing], name: str) -> None: r"""Take note of the name to which the descriptor is assigned. @@ -58,6 +81,21 @@ def __set_name__(self, owner: type[Thing], name: str) -> None: # Remember the name to which we're assigned. Accessed by the read only # property ``name``. self._name = name + self._set_name_called = True + + def assert_set_name_called(self): + """Raise an exception if ``__set_name__`` has not yet been called. + + :raises DescriptorNotAddedToClassError: if ``__set_name__`` has not yet + been called. + """ + if not self._set_name_called: + raise DescriptorNotAddedToClassError( + f"{self.__class__.__name__} must be assigned to an attribute of " + "a class, as part of the class definition. This exception is " + "raised because `__set_name__` has not yet been called, which " + "usually means it was not instantiated as a class attribute." + ) @property def name(self) -> str: @@ -74,8 +112,10 @@ def name(self) -> str: :raises NameNotSetError: if it is accessed before the descriptor has been notified of its name. """ - if self._name is None: - raise NameNotSetError + self.assert_set_name_called() + assert self._name is not None + # The assert statement is mostly for typing: if assert_set_name_called + # doesn't raise an error, self._name has been set. return self._name @property diff --git a/src/labthings_fastapi/exceptions.py b/src/labthings_fastapi/exceptions.py index e0ecb724..3e050546 100644 --- a/src/labthings_fastapi/exceptions.py +++ b/src/labthings_fastapi/exceptions.py @@ -21,7 +21,7 @@ def __init__(self, message: str | None = None): :param message: the optional message. """ - # type: ignore[call-arg] is used here because mypy can't know + # We ignore call-arg within this function because mypy can't know # that this is a mixin, and super() will be an exception (which does # accept a string argument to `__init__`). doc = inspect.cleandoc(self.__doc__) if self.__doc__ else None diff --git a/src/labthings_fastapi/thing.py b/src/labthings_fastapi/thing.py index 612c0ddc..bfa73a43 100644 --- a/src/labthings_fastapi/thing.py +++ b/src/labthings_fastapi/thing.py @@ -21,7 +21,7 @@ from pydantic import BaseModel -from .thing_property import BaseProperty, ThingSetting +from .thing_property import DataProperty, ThingSetting from .descriptors import ActionDescriptor from .thing_description._model import ThingDescription, NoSecurityScheme from .utilities import class_attributes @@ -348,7 +348,7 @@ def observe_property(self, property_name: str, stream: ObjectSendStream) -> None :raise KeyError: if the requested name is not defined on this Thing. """ prop = getattr(self.__class__, property_name) - if not isinstance(prop, BaseProperty): + if not isinstance(prop, DataProperty): raise KeyError(f"{property_name} is not a LabThings Property") prop._observers_set(self).add(stream) diff --git a/src/labthings_fastapi/thing_property.py b/src/labthings_fastapi/thing_property.py index 3f149bd5..52bd21d7 100644 --- a/src/labthings_fastapi/thing_property.py +++ b/src/labthings_fastapi/thing_property.py @@ -60,17 +60,16 @@ class attribute. Documentation is in strings immediately following the from weakref import WeakSet from fastapi import Body, FastAPI -from pydantic import RootModel +from pydantic import BaseModel, RootModel -from labthings_fastapi.thing_description import type_to_dataschema -from labthings_fastapi.thing_description._model import ( +from .thing_description import type_to_dataschema +from .thing_description._model import ( DataSchema, Form, PropertyAffordance, PropertyOp, ) -from labthings_fastapi.utilities import labthings_data - +from .utilities import labthings_data, wrap_plain_types_in_rootmodel from .utilities.introspection import return_type from .base_descriptor import BaseDescriptor from .exceptions import ( @@ -120,7 +119,7 @@ class MissingTypeError(DocstringToMessage, TypeError): # fmt: off Value = TypeVar("Value") if TYPE_CHECKING: - ValueFactory = Callable[[None,], Value] + ValueFactory = Callable[[], Value] ValueGetter = Callable[[Thing,], Value] ValueSetter = Callable[[Thing, Value], None] # fmt: on @@ -132,7 +131,9 @@ def property(default: ValueGetter) -> FunctionalProperty[Value]: ... # noqa: D1 @overload # use as `field: int = property(0)`` def property(default: Value, *, readonly: bool = False) -> Value: ... # noqa: D103 @overload # use as `field: int = property(default_factory=lambda: 0)` -def property(default_factory: ValueFactory, readonly: bool = False) -> Value: ... # noqa: D103 +def property( + *, default_factory: Callable[[], Value], readonly: bool = False +) -> Value: ... # noqa: D103 def property( @@ -206,7 +207,7 @@ def property( # If the default is callable, we're being used as a decorator # without arguments. func = default - return FunctionalProperty[return_type(func)]( + return FunctionalProperty( fget=func, ) return DataProperty( # type: ignore[return-value] @@ -224,9 +225,107 @@ class BaseProperty(BaseDescriptor[Value], Generic[Value]): means the value should be available over HTTP). `.BaseProperty` should not be used directly, instead it is recommended to - use `.property` to declare properties on you `.Thing` subclass. + use `.property` to declare properties on your `.Thing` subclass. """ + def __init__(self) -> None: + """Initialise a BaseProperty.""" + super().__init__() + self._type: type | None = None + self._model: type[BaseModel] | None = None + self.readonly: bool = False + + @builtins.property + def value_type(self) -> type[Value]: + """The type of this descriptor's value.""" + if self._type is None: + raise MissingTypeError("This property does not have a valid type.") + return self._type + + @builtins.property + def model(self) -> type[BaseModel]: + """A Pydantic model for the property's type.""" + if self._model is None: + self._model = wrap_plain_types_in_rootmodel(self.value_type) + return self._model + + def add_to_fastapi(self, app: FastAPI, thing: Thing) -> None: + """Add this action to a FastAPI app, bound to a particular Thing. + + :param app: The FastAPI application we are adding endpoints to. + :param thing: The `.Thing` we are adding the endpoints for. + """ + # We can't use the decorator in the usual way, because we'd need to + # annotate the type of `body` with `self.model` which is only defined + # at runtime. + # The solution below is to manually add the annotation, before passing + # the function to the decorator. + if not self.readonly: + + def set_property(body): # We'll annotate body later + if isinstance(body, RootModel): + body = body.root + return self.__set__(thing, body) + + set_property.__annotations__["body"] = Annotated[self.model, Body()] + assert thing.path is not None + app.put( + thing.path + self.name, + status_code=201, + response_description="Property set successfully", + summary=f"Set {self.title}", + description=f"## {self.title}\n\n{self.description or ''}", + )(set_property) + + @app.get( + thing.path + self.name, + response_model=self.model, + response_description=f"Value of {self.name}", + summary=self.title, + description=f"## {self.title}\n\n{self.description or ''}", + ) + def get_property(): + return self.__get__(thing) + + def property_affordance( + self, thing: Thing, path: str | None = None + ) -> PropertyAffordance: + """Represent the property in a Thing Description. + + :param thing: the `.Thing` to which we are attached. + :param path: the URL of the `.Thing`. If not present, we will retrieve + the ``path`` from ``thing``. + + :return: A description of the property in :ref:`wot_td` format. + """ + path = path or thing.path + ops = [PropertyOp.readproperty] + if not self.readonly: + ops.append(PropertyOp.writeproperty) + forms = [ + Form[PropertyOp]( + href=path + self.name, + op=ops, + ), + ] + data_schema: DataSchema = type_to_dataschema(self.model) + pa: PropertyAffordance = PropertyAffordance( + title=self.title, + forms=forms, + description=self.description, + ) + # We merge the data schema with the property affordance (which subclasses the + # DataSchema model) with the affordance second so its values take priority. + # Note that this works because all of the fields that get filled in by + # DataSchema are optional - so the PropertyAffordance is still valid without + # them. + return PropertyAffordance( + **{ + **data_schema.model_dump(exclude_none=True), + **pa.model_dump(exclude_none=True), + } + ) + class DataProperty(BaseProperty[Value], Generic[Value]): """A Property descriptor that acts like a regular variable. @@ -276,17 +375,20 @@ def __init__( factory function are specified. :raises MissingDefaultError: if no default is provided. """ + super().__init__() if default_factory is not None and default is not ...: raise OverspecifiedDefaultError() if default_factory is None and default is ...: raise MissingDefaultError() # The default value will come from whichever of `default` and `default_factory` # is specified. The two checks above ensure that exactly one will exist. - self._default_value: Value - if default_factory: - self._default_value = default_factory - else: - self._default_value = default + if default_factory is None: + assert default is not ... # Already checked above, this is for mypy. + + def default_factory() -> Value: + return default + + self._default_factory: ValueFactory = default_factory self.readonly = readonly self._type: type | None = None # Will be set in __set_name__ @@ -350,6 +452,12 @@ class MyThing(Thing): f"No type hint was found for property {name} on {owner}." ) + @builtins.property + def value_type(self) -> type[Value]: + """The type of the descriptor's value.""" + self.assert_set_name_called() + return super().value_type + def instance_get(self, obj: Thing) -> Value: """Return the property's value. @@ -358,10 +466,11 @@ def instance_get(self, obj: Thing) -> Value: :param obj: The `.Thing` on which the property is being accessed. :return: the value of the property. """ - try: - return obj.__dict__[self.name] - except KeyError: - return self._default_value + if self.name not in obj.__dict__: + # Note that a static default is converted to a factory function + # in __init__. + obj.__dict__[self.name] = self._default_factory() + return obj.__dict__[self.name] def __set__(self, obj: Thing, value: Value) -> None: """Set the property's value. @@ -432,82 +541,6 @@ async def emit_changed_event_async(self, obj: Thing, value: Value): {"messageType": "propertyStatus", "data": {self._name: value}} ) - def add_to_fastapi(self, app: FastAPI, thing: Thing) -> None: - """Add this action to a FastAPI app, bound to a particular Thing. - - :param app: The FastAPI application we are adding endpoints to. - :param thing: The `.Thing` we are adding the endpoints for. - """ - # We can't use the decorator in the usual way, because we'd need to - # annotate the type of `body` with `self.model` which is only defined - # at runtime. - # The solution below is to manually add the annotation, before passing - # the function to the decorator. - if not self.readonly: - - def set_property(body): # We'll annotate body later - if isinstance(body, RootModel): - body = body.root - return self.__set__(thing, body) - - set_property.__annotations__["body"] = Annotated[self.model, Body()] - app.put( - thing.path + self.name, - status_code=201, - response_description="Property set successfully", - summary=f"Set {self.title}", - description=f"## {self.title}\n\n{self.description or ''}", - )(set_property) - - @app.get( - thing.path + self.name, - response_model=self.model, - response_description=f"Value of {self.name}", - summary=self.title, - description=f"## {self.title}\n\n{self.description or ''}", - ) - def get_property(): - return self.__get__(thing) - - def property_affordance( - self, thing: Thing, path: str | None = None - ) -> PropertyAffordance: - """Represent the property in a Thing Description. - - :param thing: the `.Thing` to which we are attached. - :param path: the URL of the `.Thing`. If not present, we will retrieve - the ``path`` from ``thing``. - - :return: A description of the property in :ref:`wot_td` format. - """ - path = path or thing.path - ops = [PropertyOp.readproperty] - if not self.readonly: - ops.append(PropertyOp.writeproperty) - forms = [ - Form[PropertyOp]( - href=path + self.name, - op=ops, - ), - ] - data_schema: DataSchema = type_to_dataschema(self.model) - pa: PropertyAffordance = PropertyAffordance( - title=self.title, - forms=forms, - description=self.description, - ) - # We merge the data schema with the property affordance (which subclasses the - # DataSchema model) with the affordance second so its values take priority. - # Note that this works because all of the fields that get filled in by - # DataSchema are optional - so the PropertyAffordance is still valid without - # them. - return PropertyAffordance( - **{ - **data_schema.model_dump(exclude_none=True), - **pa.model_dump(exclude_none=True), - } - ) - def setting( default: Value | EllipsisType = ..., @@ -605,11 +638,9 @@ def set_without_emit(self, obj: Thing, value: Value): :param value: the new value of the setting. """ obj.__dict__[self.name] = value - if self._setter: - self._setter(obj, value) -class FunctionalProperty(BaseProperty[Value], Generic[Value], builtins.property): +class FunctionalProperty(BaseProperty[Value], Generic[Value]): """A property that uses a getter and a setter. For properties that should work like variables, use `.DataProperty`. For @@ -632,7 +663,9 @@ def __init__( :param fget: the getter function, called when the property is read. """ + super().__init__() self._fget: ValueGetter = fget + self._type = return_type(self._fget) self._fset: ValueSetter | None = None self.readonly: bool = True @@ -667,8 +700,9 @@ def getter(self, fget: ValueGetter) -> Self: :return: this descriptor (i.e. ``self``). This allows use as a decorator. """ self._fget = fget - if fget.__doc__: - self.__doc__ = fget.__doc__ + self._type = return_type(self._fget) + self.__doc__ = fget.__doc__ + return self def setter(self, fset: ValueSetter) -> Self: """Set the setter function of the property. @@ -701,8 +735,9 @@ def myprop(self, val: int): """ self._fset = fset self.readonly = False + return self - def deleter(self, fdel: callable) -> Self: + def deleter(self, fdel: Callable) -> Self: """Set a deleter function. Currently unsupported. :param fdel: The function called when the attribute is deleted. @@ -730,6 +765,6 @@ def __set__(self, obj: Thing, value: Value): :raises ReadOnlyPropertyError: if the property cannot be set. """ - if self.fset: - self.fset(obj, value) - raise ReadOnlyPropertyError(f"Property {self.name} of {obj} has no setter.") + if self.fset is None: + raise ReadOnlyPropertyError(f"Property {self.name} of {obj} has no setter.") + self.fset(obj, value) diff --git a/tests/test_properties.py b/tests/test_properties.py index 05dd0f50..a5f9e5d1 100644 --- a/tests/test_properties.py +++ b/tests/test_properties.py @@ -53,17 +53,30 @@ def test_instantiation_with_type(): To send the data over HTTP LabThings-FastAPI uses Pydantic models to describe data types. """ - prop: bool = lt.property(False) - assert issubclass(prop.model, BaseModel) + # This will not work unless the property is assigned to a thing + class TestThing(lt.Thing): + prop: bool = lt.property(False) -def test_instantiation_with_model(): + assert issubclass(TestThing.prop.model, BaseModel) + + +def test_instantiation_with_model() -> None: class MyModel(BaseModel): a: int = 1 b: float = 2.0 - prop: MyModel = lt.property(MyModel()) - assert prop.model is MyModel + class Dummy: + prop: MyModel = lt.property(MyModel()) + + @lt.property + def func_prop(self) -> MyModel: + return MyModel() + + assert Dummy.prop.model is MyModel # type: ignore[attr-defined] + # Dummy.prop is typed as MyModel, but it's a descriptor + + assert Dummy.func_prop.model is MyModel def test_property_get_and_set(): From a4e44d4b782f01bd0966b89e04928d1639ce36ec Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Fri, 25 Jul 2025 23:27:11 +0100 Subject: [PATCH 06/59] Fix last remaining mypy error --- src/labthings_fastapi/thing_property.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/labthings_fastapi/thing_property.py b/src/labthings_fastapi/thing_property.py index 52bd21d7..a83ae081 100644 --- a/src/labthings_fastapi/thing_property.py +++ b/src/labthings_fastapi/thing_property.py @@ -255,6 +255,7 @@ def add_to_fastapi(self, app: FastAPI, thing: Thing) -> None: :param app: The FastAPI application we are adding endpoints to. :param thing: The `.Thing` we are adding the endpoints for. """ + assert thing.path is not None # We can't use the decorator in the usual way, because we'd need to # annotate the type of `body` with `self.model` which is only defined # at runtime. @@ -268,7 +269,6 @@ def set_property(body): # We'll annotate body later return self.__set__(thing, body) set_property.__annotations__["body"] = Annotated[self.model, Body()] - assert thing.path is not None app.put( thing.path + self.name, status_code=201, From 95f44ab2b55851e94c83eb56bf91ca30b9e38d28 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 28 Jul 2025 14:06:02 +0100 Subject: [PATCH 07/59] Fix a typo in example code Thanks @julianstirling for spotting that one. --- src/labthings_fastapi/thing_property.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/labthings_fastapi/thing_property.py b/src/labthings_fastapi/thing_property.py index a83ae081..eb3fc039 100644 --- a/src/labthings_fastapi/thing_property.py +++ b/src/labthings_fastapi/thing_property.py @@ -23,7 +23,7 @@ class Counter(lt.Thing): @lt.property def remaining(self) -> int: "The number of steps remaining." - return self.remaining - self.count + return self.target - self.count @remaining.setter def remaining(self, value: int) -> None: From 2212e66fd7bb65cc5a21ec07b765181036d01f79 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Tue, 29 Jul 2025 14:55:39 +0100 Subject: [PATCH 08/59] Restore functional settings I had hoped to limit settings to a subclass of Data Property, however this would require downstream changes. I've now reimplemented them (and added a couple of tests). I've created #159 to discuss whether FunctionalSetting is a good idea long-term. I've also fixed a type error by asserting we won't generate property affordances without a valid path. --- src/labthings_fastapi/__init__.py | 4 +- src/labthings_fastapi/base_descriptor.py | 8 +- src/labthings_fastapi/thing.py | 10 +- src/labthings_fastapi/thing_property.py | 351 +++++++++++++++-------- tests/test_settings.py | 74 ++++- 5 files changed, 318 insertions(+), 129 deletions(-) diff --git a/src/labthings_fastapi/__init__.py b/src/labthings_fastapi/__init__.py index e966d170..20da7f5a 100644 --- a/src/labthings_fastapi/__init__.py +++ b/src/labthings_fastapi/__init__.py @@ -20,7 +20,7 @@ """ from .thing import Thing -from .thing_property import property, setting, DataProperty, ThingSetting +from .thing_property import property, setting, DataProperty, DataSetting from .decorators import ( thing_action, fastapi_endpoint, @@ -43,7 +43,7 @@ "property", "setting", "DataProperty", - "ThingSetting", + "DataSetting", "thing_action", "fastapi_endpoint", "deps", diff --git a/src/labthings_fastapi/base_descriptor.py b/src/labthings_fastapi/base_descriptor.py index 2192407e..eb0a8f77 100644 --- a/src/labthings_fastapi/base_descriptor.py +++ b/src/labthings_fastapi/base_descriptor.py @@ -104,13 +104,13 @@ def name(self) -> str: When the descriptor is assigned to an attribute of a class, we remember the name of the attribute. There will be some time in between the descriptor being instantiated and the name being set. - If this is accessed before we know the name, we will raise an exception. + + We call `.BaseDescriptor.assert_set_name_called` so an exception will + be raised if this property is accessed before the descriptor has been + assigned to a class attribute. The ``name`` of :ref:`wot_affordances` is used in their URL and in the :ref:`gen_docs` served by LabThings. - - :raises NameNotSetError: if it is accessed before the descriptor - has been notified of its name. """ self.assert_set_name_called() assert self._name is not None diff --git a/src/labthings_fastapi/thing.py b/src/labthings_fastapi/thing.py index bfa73a43..fd03a0c3 100644 --- a/src/labthings_fastapi/thing.py +++ b/src/labthings_fastapi/thing.py @@ -21,7 +21,7 @@ from pydantic import BaseModel -from .thing_property import DataProperty, ThingSetting +from .thing_property import DataProperty, BaseSetting from .descriptors import ActionDescriptor from .thing_description._model import ThingDescription, NoSecurityScheme from .utilities import class_attributes @@ -157,14 +157,14 @@ async def websocket(ws: WebSocket): # A private variable to hold the list of settings so it doesn't need to be # iterated through each time it is read - _settings_store: Optional[dict[str, ThingSetting]] = None + _settings_store: Optional[dict[str, BaseSetting]] = None @property - def _settings(self) -> dict[str, ThingSetting]: + def _settings(self) -> dict[str, BaseSetting]: """A private property that returns a dict of all settings for this Thing. Each dict key is the name of the setting, the corresponding value is the - ThingSetting class (a descriptor). This can be used to directly get the + BaseSetting class (a descriptor). This can be used to directly get the descriptor so that the value can be set without emitting signals, such as on startup. """ @@ -173,7 +173,7 @@ def _settings(self) -> dict[str, ThingSetting]: self._settings_store = {} for name, attr in class_attributes(self): - if isinstance(attr, ThingSetting): + if isinstance(attr, BaseSetting): self._settings_store[name] = attr return self._settings_store diff --git a/src/labthings_fastapi/thing_property.py b/src/labthings_fastapi/thing_property.py index eb3fc039..79da0cd3 100644 --- a/src/labthings_fastapi/thing_property.py +++ b/src/labthings_fastapi/thing_property.py @@ -82,6 +82,19 @@ class attribute. Documentation is in strings immediately following the from .thing import Thing +# Note on ignored linter codes: +# D103 refers to missing docstrings. I have ignored this on @overload definitions +# because they shouldn't have docstrings - the docstring belongs only on the +# function they overload. +# E302 refers to whitespace around function definitions. I have ignored this +# between @overload definitions and the function they overload, because +# it's clearer to keep them all together. Otherwise, the @overload block is easy +# to miss. +# DOC201 and D401 are ignored on properties. Because we are overriding the +# builtin `property`, we are using `@builtins.property` which is not recognised +# by pydoclint as a property. I've therefore ignored those codes manually. + + # The following exceptions are raised only when creating/setting up properties. class OverspecifiedDefaultError(DocstringToMessage, ValueError): """The default value has been specified more than once. @@ -125,18 +138,16 @@ class MissingTypeError(DocstringToMessage, TypeError): # fmt: on -# D103 ignores missing docstrings on overloads. This shouldn't be raised on overloads. +# See comment at the top of the file regarding ignored linter rules. @overload # use as a decorator @property def property(default: ValueGetter) -> FunctionalProperty[Value]: ... # noqa: D103 @overload # use as `field: int = property(0)`` def property(default: Value, *, readonly: bool = False) -> Value: ... # noqa: D103 -@overload # use as `field: int = property(default_factory=lambda: 0)` -def property( +@overload # use as `field: int = property(default_factory=lambda: 0)` # noqa: E302 +def property( # noqa: D103 *, default_factory: Callable[[], Value], readonly: bool = False -) -> Value: ... # noqa: D103 - - -def property( +) -> Value: ... +def property( # noqa: E302 default: Value | ValueGetter | EllipsisType = ..., *, default_factory: ValueFactory | None = None, @@ -237,14 +248,29 @@ def __init__(self) -> None: @builtins.property def value_type(self) -> type[Value]: - """The type of this descriptor's value.""" + """The type of this descriptor's value. + + :raises MissingTypeError: if the type has not been set. + :return: the type of the descriptor's value. + """ # noqa: D401 # see note at the top of the file if self._type is None: raise MissingTypeError("This property does not have a valid type.") return self._type @builtins.property def model(self) -> type[BaseModel]: - """A Pydantic model for the property's type.""" + """A Pydantic model for the property's type. + + `pydantic` models are used to serialise and deserialise values from + and to JSON. If the property is defined with a type hint that is not + a `pydantic.BaseModel` subclass, this property will ensure it is + wrapped in a `pydantic.RootModel` so it can be used with FastAPI. + + If `.BaseProperty.value_type` is already a `pydantic.BaseModel` + subclass, this returns it unchanged. + + :return: a Pydantic model for the property's type. + """ # noqa: D401 # see note at the top of the file if self._model is None: self._model = wrap_plain_types_in_rootmodel(self.value_type) return self._model @@ -299,6 +325,7 @@ def property_affordance( :return: A description of the property in :ref:`wot_td` format. """ path = path or thing.path + assert path is not None, "Cannot create a property affordance without a path" ops = [PropertyOp.readproperty] if not self.readonly: ops.append(PropertyOp.writeproperty) @@ -453,8 +480,8 @@ class MyThing(Thing): ) @builtins.property - def value_type(self) -> type[Value]: - """The type of the descriptor's value.""" + def value_type(self) -> type[Value]: # noqa: DOC201 + """The type of the descriptor's value.""" # noqa: D401 self.assert_set_name_called() return super().value_type @@ -472,16 +499,20 @@ def instance_get(self, obj: Thing) -> Value: obj.__dict__[self.name] = self._default_factory() return obj.__dict__[self.name] - def __set__(self, obj: Thing, value: Value) -> None: + def __set__( + self, obj: Thing, value: Value, emit_changed_event: bool = True + ) -> None: """Set the property's value. This sets the property's value, and notifies any observers. :param obj: the `.Thing` to which we are attached. :param value: the new value for the property. + :param emit_changed_event: whether to emit a changed event. """ obj.__dict__[self.name] = value - self.emit_changed_event(obj, value) + if emit_changed_event: + self.emit_changed_event(obj, value) def _observers_set(self, obj: Thing): """Return the observers of this property. @@ -542,104 +573,6 @@ async def emit_changed_event_async(self, obj: Thing, value: Value): ) -def setting( - default: Value | EllipsisType = ..., - *, - default_factory: ValueFactory | None = None, - readonly: bool = False, -) -> ThingSetting[Value]: - r"""Define a Setting on a `.Thing`\ . - - A setting is a property that is saved to disk - - This function defines a setting, which is a special Property that will - be saved to disk, so it persists even when the LabThings server is - restarted. It is otherwise very similar to `.property` with the exception - that it may only be used as a field, i.e. not as a decorator. Settings may not - be implemented with getter and setter methods, as this can conflict - with loading and saving to disk. - - A type annotation is required, and should follow the same constraints as - for :deco:`.property`. - - If the type is a pydantic BaseModel, then the setter must also be able to accept - the dictionary representation of this BaseModel as this is what will be used to - set the Setting when loading from disk on starting the server. - - .. note:: - If a setting is mutated rather than set, this will not trigger saving. - For example: if a Thing has a setting called ``dictsetting`` holding the - dictionary ``{"a": 1, "b": 2}`` then ``self.dictsetting = {"a": 2, "b": 2}`` - would trigger saving but ``self.dictsetting[a] = 2`` would not, as the - setter for ``dictsetting`` is never called. - - :param default: is the default value. Either this or - ``default_factory`` must be specified. - :param default_factory: should return your default value. - This may be used as an alternative to ``default`` if you - need to use a mutable datatype. For example, it would be - better to specify ``default_factory=list`` than - ``default=[]`` because the second form would be shared - between all `.Thing`\ s with this setting. - :param readonly: whether the setting should be read-only - via the `.ThingClient` interface (i.e. over HTTP or via - a `.DirectThingClient`). - - :return: a setting descriptor. - - **Typing Notes** - - The return type of this function is a "white lie" in order to allow - dataclass-style type annotations - """ - return ThingSetting( - default=default, - default_factory=default_factory, - readonly=readonly, - ) - - -class ThingSetting(DataProperty[Value], Generic[Value]): - """A `.DataProperty` that persists on disk. - - A setting can be accessed via the HTTP API and is persistent between sessions. - - A `.ThingSetting` is a `.DataProperty` with extra functionality for triggering - a `.Thing` to save its settings. - - Note: If a setting is mutated rather than assigned to, this will not trigger saving. - For example: if a Thing has a setting called `dictsetting` holding the dictionary - `{"a": 1, "b": 2}` then `self.dictsetting = {"a": 2, "b": 2}` would trigger saving - but `self.dictsetting[a] = 2` would not, as the setter for `dictsetting` is never - called. - - The setting otherwise acts just like a normal variable. - """ - - def __set__(self, obj: Thing, value: Value): - """Set the setting's value. - - This will cause the settings to be saved to disk. - - :param obj: the `.Thing` to which we are attached. - :param value: the new value of the setting. - """ - super().__set__(obj, value) - obj.save_settings() - - def set_without_emit(self, obj: Thing, value: Value): - """Set the property's value, but do not emit event to notify the server. - - This function is not expected to be used externally. It is called during - initial setup so that the setting can be set from disk before the server - is fully started. - - :param obj: the `.Thing` to which we are attached. - :param value: the new value of the setting. - """ - obj.__dict__[self.name] = value - - class FunctionalProperty(BaseProperty[Value], Generic[Value]): """A property that uses a getter and a setter. @@ -669,9 +602,6 @@ def __init__( self._fset: ValueSetter | None = None self.readonly: bool = True - # Note: DOC201 and DOC401 are ignored on these properties, as - # they only apply to functions. Pydoclint doesn't recognise these - # as properties, because we use `builtins.property` and not `property`. @builtins.property def fget(self) -> ValueGetter: # noqa: DOC201 """The getter function.""" # noqa: D401 @@ -768,3 +698,196 @@ def __set__(self, obj: Thing, value: Value): if self.fset is None: raise ReadOnlyPropertyError(f"Property {self.name} of {obj} has no setter.") self.fset(obj, value) + + +@overload # use as a decorator @setting +def setting(default: ValueGetter) -> FunctionalSetting[Value]: ... # noqa: D103 +@overload # use as `field: int = setting(0)`` +def setting(default: Value, *, readonly: bool = False) -> Value: ... # noqa: D103 +@overload # use as `field: int = setting(default_factory=lambda: 0)` # noqa: E302 +def setting( # noqa: D103 + *, default_factory: Callable[[], Value], readonly: bool = False +) -> Value: ... +def setting( # noqa: E302 + default: ValueGetter | Value | EllipsisType = ..., + *, + default_factory: ValueFactory | None = None, + readonly: bool = False, +) -> FunctionalSetting[Value] | Value: + r"""Define a Setting on a `.Thing`\ . + + A setting is a property that is saved to disk + + This function defines a setting, which is a special Property that will + be saved to disk, so it persists even when the LabThings server is + restarted. It is otherwise very similar to `.property`\ . + + A type annotation is required, and should follow the same constraints as + for :deco:`.property`. + + Every ``setting`` on a `.Thing` will be read each time the settings are + saved, which may be quite frequent. This means your getter must not take + too long to run, or have side-effects. Settings that use getters and + setters may be removed in the future pending the outcome of `#159`_. + + .. _`#159`: https://github.com/labthings/labthings-fastapi/issues/159 + + If the type is a pydantic BaseModel, then the setter must also be able to accept + the dictionary representation of this BaseModel as this is what will be used to + set the Setting when loading from disk on starting the server. + + .. note:: + If a setting is mutated rather than set, this will not trigger saving. + For example: if a Thing has a setting called ``dictsetting`` holding the + dictionary ``{"a": 1, "b": 2}`` then ``self.dictsetting = {"a": 2, "b": 2}`` + would trigger saving but ``self.dictsetting[a] = 2`` would not, as the + setter for ``dictsetting`` is never called. + + :param default: is the default value. Either this or + ``default_factory`` must be specified. + + When ``setting`` is used as a decorator, the function + being decorated is passed as the first argument, which is + why this argument also accepts callable objects. Callable + default values are not supported. If you want to set your + default value with a function, see ``default_factory``. + :param default_factory: should return your default value. + This may be used as an alternative to ``default`` if you + need to use a mutable datatype. For example, it would be + better to specify ``default_factory=list`` than + ``default=[]`` because the second form would be shared + between all `.Thing`\ s with this setting. + :param readonly: whether the setting should be read-only + via the `.ThingClient` interface (i.e. over HTTP or via + a `.DirectThingClient`). + + :return: a setting descriptor. + + **Typing Notes** + + The return type of this function is a "white lie" in order to allow + dataclass-style type annotations + """ + if callable(default): + # If the default is callable, we're being used as a decorator + # without arguments. + func = default + return FunctionalSetting( + fget=func, + ) + return DataSetting( # type: ignore[return-value] + default=default, + default_factory=default_factory, + readonly=readonly, + ) + + +class BaseSetting(BaseProperty[Value], Generic[Value]): + r"""A base class for settings. + + This is a subclass of `.BaseProperty` that is used to define settings. + It is not intended to be used directly, but via `.setting` and the + two concrete implementations: `.DataSetting` and `.FunctionalSetting`\ . + """ + + def set_without_emit(self, obj: Thing, value: Value) -> None: + """Set the setting's value without emitting an event. + + This is used to set the setting's value without notifying observers. + It is used during initialisation to set the value from disk before + the server is fully started. + + :param obj: the `.Thing` to which we are attached. + :param value: the new value of the setting. + + :raises NotImplementedError: this method should be implemented in subclasses. + """ + raise NotImplementedError("This method should be implemented in subclasses.") + + +class DataSetting(DataProperty[Value], BaseSetting[Value], Generic[Value]): + """A `.DataProperty` that persists on disk. + + A setting can be accessed via the HTTP API and is persistent between sessions. + + A `.ThingSetting` is a `.DataProperty` with extra functionality for triggering + a `.Thing` to save its settings. + + Note: If a setting is mutated rather than assigned to, this will not trigger saving. + For example: if a Thing has a setting called `dictsetting` holding the dictionary + `{"a": 1, "b": 2}` then `self.dictsetting = {"a": 2, "b": 2}` would trigger saving + but `self.dictsetting[a] = 2` would not, as the setter for `dictsetting` is never + called. + + The setting otherwise acts just like a normal variable. + """ + + def __set__( + self, obj: Thing, value: Value, emit_changed_event: bool = True + ) -> None: + """Set the setting's value. + + This will cause the settings to be saved to disk. + + :param obj: the `.Thing` to which we are attached. + :param value: the new value of the setting. + :param emit_changed_event: whether to emit a changed event. + """ + super().__set__(obj, value, emit_changed_event) + obj.save_settings() + + def set_without_emit(self, obj: Thing, value: Value) -> None: + """Set the property's value, but do not emit event to notify the server. + + This function is not expected to be used externally. It is called during + initial setup so that the setting can be set from disk before the server + is fully started. + + :param obj: the `.Thing` to which we are attached. + :param value: the new value of the setting. + """ + super().__set__(obj, value, emit_changed_event=False) + + +class FunctionalSetting(FunctionalProperty[Value], BaseSetting[Value], Generic[Value]): + """A `.FunctionalProperty` that persists on disk. + + A setting can be accessed via the HTTP API and is persistent between sessions. + + A `.ThingSetting` is a `.FunctionalProperty` with extra functionality for + triggering a `.Thing` to save its settings. + + Note: If a setting is mutated rather than assigned to, this will not trigger + saving. For example: if a Thing has a setting called ``dictsetting`` holding + the dictionary ``{"a": 1, "b": 2}`` then ``self.dictsetting = {"a": 2, "b": 2}`` + would trigger saving but ``self.dictsetting[a] = 2`` would not, as the setter + for ``dictsetting`` is never called. + + The setting otherwise acts just like a `.FunctionalProperty``, i.e. it uses a + getter and a setter function. + """ + + def __set__(self, obj: Thing, value: Value) -> None: + """Set the setting's value. + + This will cause the settings to be saved to disk. + + :param obj: the `.Thing` to which we are attached. + :param value: the new value of the setting. + """ + super().__set__(obj, value) + obj.save_settings() + + def set_without_emit(self, obj: Thing, value: Value) -> None: + """Set the property's value, but do not emit event to notify the server. + + This function is not expected to be used externally. It is called during + initial setup so that the setting can be set from disk before the server + is fully started. + + :param obj: the `.Thing` to which we are attached. + :param value: the new value of the setting. + """ + # FunctionalProperty does not emit changed events, so no special + # behaviour is needed. + super().__set__(obj, value) diff --git a/tests/test_settings.py b/tests/test_settings.py index 3b8638fb..f7cd8a45 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -11,6 +11,13 @@ class TestThing(lt.Thing): + """A test `.Thing` with some settings and actions.""" + + def __init__(self) -> None: + super().__init__() + # Initialize functional settings with default values + self._floatsetting: float = 1.0 + boolsetting: bool = lt.setting(False) "A boolean setting" @@ -20,8 +27,25 @@ class TestThing(lt.Thing): dictsetting: dict = lt.setting(default_factory=lambda: {"a": 1, "b": 2}) "A dictionary setting" - floatsetting: float = lt.setting(1.0) - "A float setting" + @lt.setting + def floatsetting(self) -> float: + """A float setting.""" + return self._floatsetting + + @floatsetting.setter + def floatsetting(self, value: float): + self._floatsetting = value + + @lt.setting + def readonlysetting(self) -> str: + """A read-only setting.""" + return "This is read-only" + + @readonlysetting.setter + def readonlysetting(self, value: str): + pass + + readonlysetting.readonly = True @lt.thing_action def toggle_boolsetting(self): @@ -52,6 +76,7 @@ def _settings_dict( "floatsetting": floatsetting, "stringsetting": stringsetting, "dictsetting": dictsetting, + "readonlysetting": "This is read-only", } @@ -73,10 +98,14 @@ def test_setting_available(thing): assert not thing.boolsetting assert thing.stringsetting == "foo" assert thing.floatsetting == 1.0 + assert thing.readonlysetting == "This is read-only" + +def test_functional_settings_save(thing, server): + """Check updated settings are saved to disk -def test_settings_save(thing, server): - """Check updated settings are saved to disk""" + ``floatsetting`` is a functional setting, we should also test + a `.DataSetting` for completeness.""" setting_file = _get_setting_file(server, "/thing") server.add_thing(thing, "/thing") # No setting file created when first added @@ -92,6 +121,43 @@ def test_settings_save(thing, server): assert json.load(file_obj) == _settings_dict(floatsetting=2.0) +def test_data_settings_save(thing, server): + """Check updated settings are saved to disk + + This uses ``intsetting`` which is a `.DataSetting` so it tests + a different code path to the functional setting above.""" + setting_file = _get_setting_file(server, "/thing") + server.add_thing(thing, "/thing") + # No setting file created when first added + assert not os.path.isfile(setting_file) + with TestClient(server.app) as client: + r = client.put("/thing/boolsetting", json=True) + assert r.status_code == 201 + r = client.get("/thing/boolsetting") + assert r.json() is True + assert os.path.isfile(setting_file) + with open(setting_file, "r", encoding="utf-8") as file_obj: + # Check settings on file match expected dictionary + assert json.load(file_obj) == _settings_dict(boolsetting=True) + + +def test_readonly_setting(thing, server): + """Check read-only settings cannot be set remotely.""" + setting_file = _get_setting_file(server, "/thing") + server.add_thing(thing, "/thing") + # No setting file created when first added + assert not os.path.isfile(setting_file) + with TestClient(server.app) as client: + # Check we can read it over HTTP + r = client.get("/thing/readonlysetting") + assert r.json() == "This is read-only" + assert r.status_code == 200 + # Attempt to set read-only setting + r = client.put("/thing/readonlysetting", json="new value") + assert r.status_code == 405 + assert not os.path.isfile(setting_file) # No file created + + def test_settings_dict_save(thing, server): """Check settings are saved if the dict is updated in full""" setting_file = _get_setting_file(server, "/thing") From 2809992691e8d8728d81c8d7c08f200d8e41777a Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Tue, 29 Jul 2025 15:30:16 +0100 Subject: [PATCH 09/59] Added a tutorial page on properties and things I would love to get doctest working for this, but I'll leave that for a future PR. --- docs/source/tutorial/index.rst | 2 + docs/source/tutorial/properties.rst | 111 +++++++++++++++++++++++ docs/source/tutorial/writing_a_thing.rst | 38 ++++++++ 3 files changed, 151 insertions(+) create mode 100644 docs/source/tutorial/properties.rst create mode 100644 docs/source/tutorial/writing_a_thing.rst diff --git a/docs/source/tutorial/index.rst b/docs/source/tutorial/index.rst index 19a2f06f..7bd46ef2 100644 --- a/docs/source/tutorial/index.rst +++ b/docs/source/tutorial/index.rst @@ -7,6 +7,8 @@ LabThings-FastAPI tutorial installing_labthings.rst running_labthings.rst + writing_a_thing.rst + properties.rst .. In due course, these pages should exist... diff --git a/docs/source/tutorial/properties.rst b/docs/source/tutorial/properties.rst new file mode 100644 index 00000000..0ce85742 --- /dev/null +++ b/docs/source/tutorial/properties.rst @@ -0,0 +1,111 @@ +.. _tutorial_properties: + +Properties +========================= + +:ref:`wot_properties` are values that can be read and written on a Thing. They are used to represent the state of the Thing, such as its current temperature, brightness, or status. Properties can be read using a ``GET`` request and written using a ``PUT`` or ``POST`` request. You can add properties to a `.Thing` by using `.property` (usually imported as ``lt.property``). + +Data properties +------------------------- + +Data properties behave like variables: they simply store a value that is used by other code on the `.Thing`. They are defined similarly to fields in `dataclasses` or `pydantic` models: + +.. code-block:: python + + import labthings_fastapi as lt + + class MyThing(lt.Thing): + my_property: int = lt.property(42) + +The example above defines a property called `my_property` that has a default value of `42`. Note the type hint `int` which indicates that the property should hold an integer value. This is important, as the type will be enforced when the property is written to via HTTP, and it will appear in :ref:`gen_docs`. By default, this property may be read or written to by HTTP requests. If you want to make it read-only, you can set the `readonly` parameter to `True`: + +.. code-block:: python + + class MyThing(lt.Thing): + my_property: int = lt.property(42, readonly=True) + +Note that the ``readonly`` parameter only affects *client* code, i.e. it may not be written to via HTTP requests or `.DirectThingClient` instances. However, the property can still be modified by the Thing's code, e.g. in response to an action or another property change as ``self.my_property = 100``. + +It is a good idea to make sure there is a docstring for your property. This will be used in the :ref:`gen_docs`, and it will help users understand what the property is for. You can add a docstring to the property by placing a string immediately after the property definition: + +.. code-block:: python + + class MyThing(lt.Thing): + my_property: int = lt.property(42, readonly=True) + """A property that holds an integer value.""" + +You don't need to include the type in the docstring, as it will be inferred from the type hint. However, you can include additional information about the property, such as its units or any constraints on its value. + +Data properties may be *observed*, which means notifications will be sent when the property is written to (see below). + +Functional properties +------------------------- + +It is also possible to have properties that run code when they are read or written to. These are called functional properties, and they are defined using the `lt.FunctionalProperty` class. They might communicate with hardware (for example to read or write a setting on an instrument), or they might perform some computation based on other properties. They are defined with a decorator, very similarly to the built-in `property` function: + +.. code-block:: python + + import labthings_fastapi as lt + + class MyThing(lt.Thing): + my_property: int = lt.property(42) + """A property that holds an integer value.""" + + @lt.property + def twice_my_property(self) -> int: + """Twice the value of my_property.""" + return self.my_property * 2 + +The example above defines a functional property called `twice_my_property` that returns twice the value of `my_property`. The type hint `-> int` indicates that the property should return an integer value. When this property is read via HTTP, the code in the method will be executed, and the result will be returned to the client. As with `property`, the docstring of the property is taken from the method's docstring, so you can include additional information about the property there. + +Functional properties may also have a "setter" method, which is called when the property is written to via HTTP. This allows you to perform some action when the property is set, such as updating a hardware setting or performing some computation. The setter method should take a single argument, which is the new value of the property: + +.. code-block:: python + + import labthings_fastapi as lt + + class MyThing(lt.Thing): + my_property: int = lt.property(42) + """A property that holds an integer value.""" + + @lt.property + def twice_my_property(self) -> int: + """Twice the value of my_property.""" + return self.my_property * 2 + + @twice_my_property.setter + def twice_my_property(self, value: int): + """Set the value of twice_my_property.""" + self.my_property = value // 2 + +Adding a setter makes the property read-write (if only a getter is present, it must be read-only). It is possible to make a property read-only for clients by setting its ``readonly`` attribute: this has the same behaviour as for data properties, i.e. it prevents the property from being written to via HTTP requests or `.DirectThingClient` instances, but it can still be modified by the Thing's code. + +.. code-block:: python + + import labthings_fastapi as lt + + class MyThing(lt.Thing): + my_property: int = lt.property(42) + """A property that holds an integer value.""" + + @lt.property + def twice_my_property(self) -> int: + """Twice the value of my_property.""" + return self.my_property * 2 + + @twice_my_property.setter + def twice_my_property(self, value: int): + """Set the value of twice_my_property.""" + self.my_property = value // 2 + + # Make the property read-only for clients + twice_my_property.readonly = True + +Functional properties may not be observed, as they are not backed by a simple value. If you need to notify clients when the value changes, you can use a data property that is updated by the functional property. + +Observable properties +------------------------- + +Properties can be made observable, which means that clients can subscribe to changes in the property's value. This is useful for properties that change frequently, such as sensor readings or instrument settings. In order for a property to be observable, LabThings must know whenever it changes. Currently, this means only data properties can be observed, as functional properties do not have a simple value that can be tracked. + +Properties are currently only observable via websockets: in the future, it may be possible to observe them from other `.Thing` instances or from other parts of the code. diff --git a/docs/source/tutorial/writing_a_thing.rst b/docs/source/tutorial/writing_a_thing.rst new file mode 100644 index 00000000..d8ad37b4 --- /dev/null +++ b/docs/source/tutorial/writing_a_thing.rst @@ -0,0 +1,38 @@ +.. tutorial_thing: + +Writing a Thing +========================= + +In this section, we will write a simple example `.Thing` that provides some functionality on the server. + +.. note:: + + Usually, you will write your own `.Thing` in a separate Python module and run it using a configuration file as described in :ref:`tutorial_running`. However, for this tutorial, we will write the `.Thing` in a single file, and use a ``__name__ == "__main__"`` block to run it directly. This is not recommended for production code, but it is convenient for a tutorial. + +Our first Thing will pretend to be a light: we can set its brightness and turn it on and off. A first, most basic implementation might look like: + +.. code-block:: python + + import labthings_fastapi as lt + + class Light(lt.Thing): + brightness: int = lt.property(100) + is_on: bool = lt.property(False, readonly=true) + + @lt.action + def toggle(self): + self.is_on = not self.is_on + + + light = Light() + server = lt.ThingServer() + server.add_thing("/light", light) + + if __name__ == "__main__": + import uvicorn + # We run the server using `uvicorn`: + uvicorn.run(server.app, port=5000) + +If you visit `http://localhost:5000/light`, you will see the Thing Description. You can also interact with it using the OpenAPI documentation at `http://localhost:5000/docs`. If you visit `http://localhost:5000/light/brightness`, you can set the brightness of the light, and if you visit `http://localhost:5000/light/is_on`, you can see whether the light is on. Changing values on the server requires a ``PUT`` or ``POST`` request, which is easiest to do using the OpenAPI "Try it out" feature. Check that you can use a ``POST`` request to the ``toggle`` endpoint to turn the light on and off. + +There are two types of :ref:`wot_affordances` in this example: properties and actions. Properties are used to read and write values, while actions are used to perform operations that change the state of the Thing. In this case, we have a property for the brightness of the light and a property to indicate whether the light is on or off. The action ``toggle`` changes the state of the light by toggling the ``is_on`` property between ``True`` and ``False``. \ No newline at end of file From 90d5de804279d6f6d9336b460ab34fa73adc1c90 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Tue, 29 Jul 2025 20:37:11 +0100 Subject: [PATCH 10/59] Add typing tests for thing definition code. Currently, mypy checks the package for type safety. It would be really useful to check that code using the library will be typed correctly, so that Thing subclasses will play nicely with mypy and friends. This commit starts that process, by adding a folder of test code that will be run through mypy, with `--warn-unused-ignores`. Activating the `unused-ignores` error means that we can check that code raises an error just by adding a # type: ignore comment. If the error isn't raised, the unused # type: ignore will cause an error. This is not yet passing, because the getter/setter form of property triggers an error, which seems to be a difficult one to work around. This has been described previously: https://github.com/python/typing/discussions/1102 --- .github/workflows/test.yml | 3 + pyproject.toml | 5 + typing_tests/README.md | 11 +++ typing_tests/thing_definitions.py | 151 ++++++++++++++++++++++++++++++ 4 files changed, 170 insertions(+) create mode 100644 typing_tests/README.md create mode 100644 typing_tests/thing_definitions.py diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 97dc8a5a..958d1f3c 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -75,6 +75,9 @@ jobs: - name: Analyse with MyPy run: mypy src + + - name: Type tests with MyPy + run: mypy --warn-unused-ignores .\typing_tests\ test-with-unpinned-deps: runs-on: ubuntu-latest diff --git a/pyproject.toml b/pyproject.toml index fbec4856..120c3ea0 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -89,12 +89,17 @@ ignore = [ preview = true [tool.ruff.lint.per-file-ignores] +# Tests are currently not fully docstring-ed, we'll ignore this for now. "tests/*" = ["D", "DOC"] +# Typing tests do have docstrings, but it's not helpful to insist on imperative +# mood etc. +"typing_tests/*" = ["D404", "D401"] "docs/*" = ["D", "DOC"] [tool.mypy] plugins = ["pydantic.mypy"] + [tool.flake8] extend-ignore = [ "DOC301", # allow class + __init__ docstrings diff --git a/typing_tests/README.md b/typing_tests/README.md new file mode 100644 index 00000000..6fd3117d --- /dev/null +++ b/typing_tests/README.md @@ -0,0 +1,11 @@ +# Typing tests: check `labthings_fastapi` plays nicely with `mypy`. + +The codebase is type-checked with `mypy src/` and tested with `pytest`, however neither of these explicitly check that `mypy` can infer the correct types for `Thing` attributes like properties and actions. The Python files in this folder are intended to be checked using: + +```terminal +mypy --warn-unused-ignores .\typing_tests\ +``` + +The files include valid code that's accompanied by `assert_type` statements (which check the inferred types are what we expect them to be) as well as invalid code where the expected `mypy` errors are ignored. This tests for expected errors - if an expected error is not thrown, it will cause an `unused-ignore` error. + +There are more elaborate type testing solutions available, but all of them add dependencies and require us to learn a new tool. This folder of "tests" feels like a reasonable way to test that the package plays well with static type checking, without too much added complication. \ No newline at end of file diff --git a/typing_tests/thing_definitions.py b/typing_tests/thing_definitions.py new file mode 100644 index 00000000..9feb93da --- /dev/null +++ b/typing_tests/thing_definitions.py @@ -0,0 +1,151 @@ +"""Test thing definitions for type checking. + +This module checks that code defining a Thing may be type checked using +mypy. + +See README.md for more details. +""" + +import labthings_fastapi as lt + +from typing import assert_type + + +def optional_int_factory() -> int | None: + """Return an optional int.""" + return None + + +def int_factory() -> int: + """Return an int.""" + return 0 + + +class TestPropertyDefaultsMatch(lt.Thing): + """A Thing that checks our property type hints are working. + + This Thing defines properties in various ways. Some of these should cause + mypy to throw errors, for example if the default has the wrong type. + """ + + # These properties should not cause mypy errors, as the default matches + # the type hint. + intprop: int = lt.property(0) + optionalintprop: int | None = lt.property(None) + optionalintprop2: int | None = lt.property(0) + optionalintprop3: int | None = lt.property(default_factory=optional_int_factory) + + # This property should cause mypy to throw an error, as the default is a string. + # The type hint is an int, so this should cause a mypy error. + intprop2: int = lt.property("foo") # type: ignore[assignment] + intprop3: int = lt.property(default_factory=optional_int_factory) # type: ignore[assignment] + + # Data properies must always have a default, so this line should fail + # with mypy. It will also raise an excetion at runtime, and there's a + # test for that run with pytest. + intprop4: int = lt.property() # type: ignore[call-overload] + "This property should cause mypy to throw an error, as it has no default." + + listprop: list[int] = lt.property(default_factory=list) + "A list property with a default factory. Note the default factory is a less specific type." + + +# Check that the type hints on an instance of the class are correct. +test_defaults_match = TestPropertyDefaultsMatch() +assert_type(test_defaults_match.intprop, int) +assert_type(test_defaults_match.intprop2, int) +assert_type(test_defaults_match.intprop3, int) +assert_type(test_defaults_match.optionalintprop, int | None) +assert_type(test_defaults_match.optionalintprop2, int | None) +assert_type(test_defaults_match.optionalintprop3, int | None) + +# NB the types of the class attributes will be the same as the instance attributes +# because of the type hint on `lt.property`. This is incorrect (the class attributes +# will be `DataProperty` instances), but it is not something that code outside of +# LabThings-FastAPI should rely on. See typing notes in `lt.property` docstring +# for more details. + + +class TestExplicitDescriptor(lt.Thing): + """A Thing that checks our explicit descriptor type hints are working.""" + + intprop1 = lt.DataProperty[int](0) + """A DataProperty that should not cause mypy errors.""" + + intprop2 = lt.DataProperty[int](default_factory=int_factory) + """This property should not cause mypy errors, as the factory matches the type hint.""" + + intprop3 = lt.DataProperty[int](default_factory=optional_int_factory) + """Uses a factory function that doesn't match the type hint. + + This ought to cause mypy to throw an error, as the factory function can + return None, but at time of writing this doesn't happen. + + This error is caught correctly when called via `lt.property`. + """ + + intprop4 = lt.DataProperty[int]("foo") # type: ignore[call-overload] + """This property should cause mypy to throw an error, as the default is a string.""" + + intprop5 = lt.DataProperty[int]() # type: ignore[call-overload] + """This property should cause mypy to throw an error, as it has no default.""" + + optionalintprop1 = lt.DataProperty[int | None](None) + """A DataProperty that should not cause mypy errors.""" + + optionalintprop2 = lt.DataProperty[int | None](default_factory=optional_int_factory) + """This property should not cause mypy errors, as the factory matches the type hint.""" + + optionalintprop3 = lt.DataProperty[int | None](default_factory=int_factory) + """Uses a factory function that is a subset of the type hint.""" + + +# Check instance attributes are typed correctly. +test_explicit_descriptor = TestExplicitDescriptor() +assert_type(test_explicit_descriptor.intprop1, int) +assert_type(test_explicit_descriptor.intprop2, int) +assert_type(test_explicit_descriptor.intprop3, int) + +assert_type(test_explicit_descriptor.optionalintprop1, int | None) +assert_type(test_explicit_descriptor.optionalintprop2, int | None) +assert_type(test_explicit_descriptor.optionalintprop3, int | None) + +# Check class attributes are typed correctly. +assert_type(TestExplicitDescriptor.intprop1, lt.DataProperty[int]) +assert_type(TestExplicitDescriptor.intprop2, lt.DataProperty[int]) +assert_type(TestExplicitDescriptor.intprop3, lt.DataProperty[int]) + +assert_type(TestExplicitDescriptor.optionalintprop1, lt.DataProperty[int | None]) +assert_type(TestExplicitDescriptor.optionalintprop2, lt.DataProperty[int | None]) +assert_type(TestExplicitDescriptor.optionalintprop3, lt.DataProperty[int | None]) + + +class TestFunctionalProperty(lt.Thing): + """A Thing that checks our functional property type hints are working.""" + + @lt.property + def intprop1(self) -> int: + """A functional property that should not cause mypy errors.""" + return 0 + + @lt.property + def intprop2(self) -> int: + """This property should not cause mypy errors and is writeable.""" + return 0 + + @intprop2.setter + def intprop2(self, value: int): + """Setter for intprop2.""" + pass + + intprop2.readonly = True # This should be an OK thing to do. + + @lt.property + def intprop3(self) -> int: + """Cause mypy to throw an error as the setter is wrongly typed.""" + return 0 + + @intprop3.setter + def intprop3(self, value: str): + """Setter for intprop3.""" + pass From 847c192c1ed03b9cb3b70a6f0c8c4111453a1d1b Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Tue, 29 Jul 2025 21:05:51 +0100 Subject: [PATCH 11/59] Tighten up type checking of properties I've used overloads to make it clear to mypy that exactly one of `default` and `default_factory` is required. This ended up duplicating some logic in `DataProperty.__init__`, `property`, and `setting`, so I've consolidated that into a function. --- src/labthings_fastapi/thing_property.py | 87 ++++++++++++++++++------- 1 file changed, 64 insertions(+), 23 deletions(-) diff --git a/src/labthings_fastapi/thing_property.py b/src/labthings_fastapi/thing_property.py index 79da0cd3..1197f724 100644 --- a/src/labthings_fastapi/thing_property.py +++ b/src/labthings_fastapi/thing_property.py @@ -83,9 +83,11 @@ class attribute. Documentation is in strings immediately following the # Note on ignored linter codes: +# # D103 refers to missing docstrings. I have ignored this on @overload definitions # because they shouldn't have docstrings - the docstring belongs only on the # function they overload. +# D105 is the same as D103, but for __init__ (i.e. magic methods). # E302 refers to whitespace around function definitions. I have ignored this # between @overload definitions and the function they overload, because # it's clearer to keep them all together. Otherwise, the @overload block is easy @@ -138,6 +140,54 @@ class MissingTypeError(DocstringToMessage, TypeError): # fmt: on +def default_factory_from_arguments( + default: Value | EllipsisType = ..., + default_factory: ValueFactory | None = None, +) -> ValueFactory: + """Process default arguments to get a default factory function. + + This function takes the ``default`` and ``default_factory`` arguments + and will either return the ``default_factory`` if it is provided, or + will wrap the default value provided in a factory function. + + This is used to avoid repeating the logic of checking whether a default + value or a factory function has been provided, and it returns a factory + rather than a default value so that it may be called multiple times to + get copies of the default value. + + This function also ensures the default is specified exactly once, and + raises exceptions if it is not. + + This logic originally lived only in the initialiser of `.DataProperty` + but it was needed in the `.property` and `.setting` functions in order + to correctly type them (so that specifying both or neither of the + ``default`` and ``default_factory`` arguments would raise an error + with mypy). + + :param default: the default value, or an ellipsis if not specified. + :param default_factory: a function that returns the default value. + :return: a function that returns the default value. + :raises OverspecifiedDefaultError: if both ``default`` and + ``default_factory`` are specified. + :raises MissingDefaultError: if neither ``default`` nor ``default_factory`` + are specified. + """ + if default is ... and default_factory is None: + # If the default is an ellipsis, we have no default value. + # This is not allowed for DataProperty, so we raise an error. + raise MissingDefaultError() + if default is not ... and default_factory is not None: + # If both default and default_factory are set, we raise an error. + raise OverspecifiedDefaultError() + if default is not ...: + + def default_factory() -> Value: + return default + + assert callable(default_factory), "The default_factory must be callable." + return default_factory + + # See comment at the top of the file regarding ignored linter rules. @overload # use as a decorator @property def property(default: ValueGetter) -> FunctionalProperty[Value]: ... # noqa: D103 @@ -166,7 +216,8 @@ def property( # noqa: E302 or a subclass of `pydantic.BaseModel`. :param default: is the default value. Either this or - ``default_factory`` must be specified. + ``default_factory`` must be specified. Specifying both + or neither will raise an exception. When ``property`` is used as a decorator, the function being decorated is passed as the first argument, which is @@ -222,8 +273,7 @@ def property( # noqa: E302 fget=func, ) return DataProperty( # type: ignore[return-value] - default=default, - default_factory=default_factory, + default_factory=default_factory_from_arguments(default, default_factory), readonly=readonly, ) @@ -361,11 +411,17 @@ class DataProperty(BaseProperty[Value], Generic[Value]): written to like a regular Python variable. """ + @overload + def __init__(self, default: Value, *, readonly: bool = False) -> None: ... # noqa: D105 + @overload + def __init__( + self, *, default_factory: ValueFactory, readonly: bool = False + ) -> None: ... # noqa: D105 def __init__( self, default: Value | EllipsisType = ..., *, - default_factory: ValueFactory | None, + default_factory: ValueFactory | None = None, readonly: bool = False, ): """Create a property that acts like a regular variable. @@ -397,25 +453,11 @@ def __init__( :param readonly: if ``True``, the property may not be written to via HTTP, or via `.DirectThingClient` objects, i.e. it may only be set as an attribute of the `.Thing` and not from a client. - - :raises OverspecifiedDefaultError: if both a default and a default - factory function are specified. - :raises MissingDefaultError: if no default is provided. """ super().__init__() - if default_factory is not None and default is not ...: - raise OverspecifiedDefaultError() - if default_factory is None and default is ...: - raise MissingDefaultError() - # The default value will come from whichever of `default` and `default_factory` - # is specified. The two checks above ensure that exactly one will exist. - if default_factory is None: - assert default is not ... # Already checked above, this is for mypy. - - def default_factory() -> Value: - return default - - self._default_factory: ValueFactory = default_factory + self._default_factory = default_factory_from_arguments( + default=default, default_factory=default_factory + ) self.readonly = readonly self._type: type | None = None # Will be set in __set_name__ @@ -776,8 +818,7 @@ def setting( # noqa: E302 fget=func, ) return DataSetting( # type: ignore[return-value] - default=default, - default_factory=default_factory, + default_factory=default_factory_from_arguments(default, default_factory), readonly=readonly, ) From 25dbdfb205e468b2d81b81215a2a4d6c14a48617 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Tue, 29 Jul 2025 23:26:13 +0100 Subject: [PATCH 12/59] Make `default` a keyword-only argument for `setting` and `property` Typing for functional properties/settings can be fixed if we improve the type hints that are used to pick between @overload definitions. The main change is that `default` must become a keyword-only argument. --- .../example_things/__init__.py | 4 +- src/labthings_fastapi/thing_property.py | 76 ++++++++++++++---- typing_tests/thing_definitions.py | 77 ++++++++++++++++--- 3 files changed, 128 insertions(+), 29 deletions(-) diff --git a/src/labthings_fastapi/example_things/__init__.py b/src/labthings_fastapi/example_things/__init__.py index 3341a2d2..f26c1806 100644 --- a/src/labthings_fastapi/example_things/__init__.py +++ b/src/labthings_fastapi/example_things/__init__.py @@ -94,10 +94,10 @@ def slowly_increase_counter(self, increments: int = 60, delay: float = 1): time.sleep(delay) self.increment_counter() - counter: int = lt_property(0, readonly=True) + counter: int = lt_property(default=0, readonly=True) "A pointless counter" - foo: str = lt_property("Example") + foo: str = lt_property(default="Example") "A pointless string for demo purposes." @thing_action diff --git a/src/labthings_fastapi/thing_property.py b/src/labthings_fastapi/thing_property.py index 1197f724..f372bed7 100644 --- a/src/labthings_fastapi/thing_property.py +++ b/src/labthings_fastapi/thing_property.py @@ -49,9 +49,11 @@ class attribute. Documentation is in strings immediately following the from types import EllipsisType from typing import ( Annotated, + Any, Callable, Generic, Self, + TypeAlias, TypeVar, overload, TYPE_CHECKING, @@ -131,13 +133,24 @@ class MissingTypeError(DocstringToMessage, TypeError): """ -# fmt: off Value = TypeVar("Value") if TYPE_CHECKING: - ValueFactory = Callable[[], Value] - ValueGetter = Callable[[Thing,], Value] - ValueSetter = Callable[[Thing, Value], None] -# fmt: on + # It's hard to type check methods, because the type of ``self`` + # will be a subclass of `.Thing`, and `Callable` types are + # contravariant in their arguments (i.e. + # ``Callable[[SpecificThing,], Value])`` is not a subtype of + # ``Callable[[Thing,], Value]``. + # It is probably not particularly important for us to check th + # type of ``self`` when decorating methods, so it is left as + # ``Any`` to avoid the major confusion that would result from + # trying to type it more tightly. + # + # Note: in ``@overload`` definitions, it's sometimes necessary + # to avoid the use of these aliases, as ``mypy`` can't + # pick which variant is in use without the explicit `Callable`. + ValueFactory: TypeAlias = Callable[[], Value] + ValueGetter: TypeAlias = Callable[[Any], Value] + ValueSetter: TypeAlias = Callable[[Any, Value], None] def default_factory_from_arguments( @@ -190,16 +203,19 @@ def default_factory() -> Value: # See comment at the top of the file regarding ignored linter rules. @overload # use as a decorator @property -def property(default: ValueGetter) -> FunctionalProperty[Value]: ... # noqa: D103 +def property( + getter: Callable[[Any], Value], +) -> FunctionalProperty[Value]: ... # noqa: D103 @overload # use as `field: int = property(0)`` -def property(default: Value, *, readonly: bool = False) -> Value: ... # noqa: D103 +def property(*, default: Value, readonly: bool = False) -> Value: ... # noqa: D103 @overload # use as `field: int = property(default_factory=lambda: 0)` # noqa: E302 def property( # noqa: D103 *, default_factory: Callable[[], Value], readonly: bool = False ) -> Value: ... def property( # noqa: E302 - default: Value | ValueGetter | EllipsisType = ..., + getter: ValueGetter | None = None, *, + default: Value | EllipsisType = ..., default_factory: ValueFactory | None = None, readonly: bool = False, ) -> Value | FunctionalProperty[Value]: @@ -265,12 +281,11 @@ def property( # noqa: E302 distinguish between ``default`` not being set (``...``) and a desired default value of ``None``. """ - if callable(default): + if callable(getter): # If the default is callable, we're being used as a decorator # without arguments. - func = default return FunctionalProperty( - fget=func, + fget=getter, ) return DataProperty( # type: ignore[return-value] default_factory=default_factory_from_arguments(default, default_factory), @@ -423,7 +438,7 @@ def __init__( *, default_factory: ValueFactory | None = None, readonly: bool = False, - ): + ) -> None: """Create a property that acts like a regular variable. `.DataProperty` descriptors function just like variables, in that @@ -697,13 +712,39 @@ def myprop(self) -> int: return self._myprop @myprop.setter - def myprop(self, val: int): + def set_myprop(self, val: int) -> None: self._myprop = val myprop.readonly = True # Prevent client code from setting it + .. note:: + + The example code above is not quite what would be done for the built-in + ``@property`` decorator, because our setter does not have the same name + as the getter. Using a different name avoids type checkers such as + ``mypy`` raising an error that the getter has been redefined with a + different type. The behaviour is identical whether the setter and getter + have the same name or not. The only difference is that the `.Thing` + will have an additional method called ``set_myprop`` in the example + above. + :param fset: The new setter function. :return: this descriptor (i.e. ``self``). This allows use as a decorator. + + **Typing Notes** + + Python's built-in ``property`` is treated as a special case by ``mypy`` + and others, and our descriptor is not treated in the same way. + Naming the setter and getter the same is required by `builtins.property` + because the property must be overwritten when the setter is added, as + `builtins.property` is not mutable. + + Our descriptor is mutable, so the setter may be added without having to + overwrite the object. While it would be nice to use exactly the same + conventions as `builtins.property`, it currently causes type errors that + must be silenced manually. We suggest using a different name for the setter + as an alternative to adding ``# type: ignore[no-redef]`` to the setter + function. """ self._fset = fset self.readonly = False @@ -743,16 +784,19 @@ def __set__(self, obj: Thing, value: Value): @overload # use as a decorator @setting -def setting(default: ValueGetter) -> FunctionalSetting[Value]: ... # noqa: D103 +def setting( + getter: Callable[[Any], Value], +) -> FunctionalSetting[Value]: ... # noqa: D103 @overload # use as `field: int = setting(0)`` -def setting(default: Value, *, readonly: bool = False) -> Value: ... # noqa: D103 +def setting(*, default: Value, readonly: bool = False) -> Value: ... # noqa: D103 @overload # use as `field: int = setting(default_factory=lambda: 0)` # noqa: E302 def setting( # noqa: D103 *, default_factory: Callable[[], Value], readonly: bool = False ) -> Value: ... def setting( # noqa: E302 - default: ValueGetter | Value | EllipsisType = ..., + getter: ValueGetter | None = None, *, + default: Value | EllipsisType = ..., default_factory: ValueFactory | None = None, readonly: bool = False, ) -> FunctionalSetting[Value] | Value: diff --git a/typing_tests/thing_definitions.py b/typing_tests/thing_definitions.py index 9feb93da..f75662d2 100644 --- a/typing_tests/thing_definitions.py +++ b/typing_tests/thing_definitions.py @@ -7,8 +7,10 @@ """ import labthings_fastapi as lt +from labthings_fastapi.thing_property import FunctionalProperty from typing import assert_type +import typing def optional_int_factory() -> int | None: @@ -21,6 +23,29 @@ def int_factory() -> int: return 0 +unbound_prop = lt.property(default=0) +"""A property that is not bound to a Thing. + +This will go wrong at runtime if we access its ``model`` but it should +have its type inferred as an `int`. +""" +assert_type(unbound_prop, int) + +unbound_prop_2 = lt.property(default_factory=int_factory) +"""A property that is not bound to a Thing, with a factory.""" + +assert_type(unbound_prop_2, int) + + +@lt.property +def strprop(self) -> str: + """A functional property that should not cause mypy errors.""" + return "foo" + + +assert_type(strprop, FunctionalProperty[str]) + + class TestPropertyDefaultsMatch(lt.Thing): """A Thing that checks our property type hints are working. @@ -30,14 +55,14 @@ class TestPropertyDefaultsMatch(lt.Thing): # These properties should not cause mypy errors, as the default matches # the type hint. - intprop: int = lt.property(0) - optionalintprop: int | None = lt.property(None) - optionalintprop2: int | None = lt.property(0) + intprop: int = lt.property(default=0) + optionalintprop: int | None = lt.property(default=None) + optionalintprop2: int | None = lt.property(default=0) optionalintprop3: int | None = lt.property(default_factory=optional_int_factory) # This property should cause mypy to throw an error, as the default is a string. # The type hint is an int, so this should cause a mypy error. - intprop2: int = lt.property("foo") # type: ignore[assignment] + intprop2: int = lt.property(default="foo") # type: ignore[assignment] intprop3: int = lt.property(default_factory=optional_int_factory) # type: ignore[assignment] # Data properies must always have a default, so this line should fail @@ -69,7 +94,7 @@ class TestPropertyDefaultsMatch(lt.Thing): class TestExplicitDescriptor(lt.Thing): """A Thing that checks our explicit descriptor type hints are working.""" - intprop1 = lt.DataProperty[int](0) + intprop1 = lt.DataProperty[int](default=0) """A DataProperty that should not cause mypy errors.""" intprop2 = lt.DataProperty[int](default_factory=int_factory) @@ -84,13 +109,13 @@ class TestExplicitDescriptor(lt.Thing): This error is caught correctly when called via `lt.property`. """ - intprop4 = lt.DataProperty[int]("foo") # type: ignore[call-overload] + intprop4 = lt.DataProperty[int](default="foo") # type: ignore[call-overload] """This property should cause mypy to throw an error, as the default is a string.""" intprop5 = lt.DataProperty[int]() # type: ignore[call-overload] """This property should cause mypy to throw an error, as it has no default.""" - optionalintprop1 = lt.DataProperty[int | None](None) + optionalintprop1 = lt.DataProperty[int | None](default=None) """A DataProperty that should not cause mypy errors.""" optionalintprop2 = lt.DataProperty[int | None](default_factory=optional_int_factory) @@ -119,6 +144,14 @@ class TestExplicitDescriptor(lt.Thing): assert_type(TestExplicitDescriptor.optionalintprop2, lt.DataProperty[int | None]) assert_type(TestExplicitDescriptor.optionalintprop3, lt.DataProperty[int | None]) +Val = typing.TypeVar("Val") +Thing = typing.TypeVar("Thing", bound=lt.Thing) + + +def f_property(getter: typing.Callable[..., Val]) -> FunctionalProperty[Val]: + """A function that returns a DataProperty with a getter.""" + return FunctionalProperty(getter) + class TestFunctionalProperty(lt.Thing): """A Thing that checks our functional property type hints are working.""" @@ -134,7 +167,7 @@ def intprop2(self) -> int: return 0 @intprop2.setter - def intprop2(self, value: int): + def set_intprop2(self, value: int): """Setter for intprop2.""" pass @@ -142,10 +175,32 @@ def intprop2(self, value: int): @lt.property def intprop3(self) -> int: - """Cause mypy to throw an error as the setter is wrongly typed.""" + """This getter is fine, but the setter should fail type checking.""" return 0 @intprop3.setter - def intprop3(self, value: str): - """Setter for intprop3.""" + def set_intprop3(self, value: str) -> None: + """Setter for intprop3. It's got the wrong type so should fail.""" + pass + + @f_property + def fprop(self) -> int: + """A functional property that should not cause mypy errors.""" + return 0 + + @strprop.setter + def set_fprop(self, value: int) -> None: + """Setter for strprop. This should fail type checking.""" pass + + +assert_type(TestFunctionalProperty.intprop1, FunctionalProperty[int]) +assert_type(TestFunctionalProperty.intprop2, FunctionalProperty[int]) +assert_type(TestFunctionalProperty.intprop3, FunctionalProperty[int]) +assert_type(TestFunctionalProperty.fprop, FunctionalProperty[int]) + +test_functional_property = TestFunctionalProperty() +assert_type(test_functional_property.intprop1, int) +assert_type(test_functional_property.intprop2, int) +assert_type(test_functional_property.intprop3, int) +assert_type(test_functional_property.fprop, int) From a64bc244c001f0f8ca8d87bdaa42f0fc734c1a9e Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 30 Jul 2025 00:00:48 +0100 Subject: [PATCH 13/59] Re-lock dependencies under Windows, Python 3.10 I think these were last locked under Linux, and this added a dependency on `uvloop` which breaks installation on Windows. Given that CI has been passing with dependencies locked under Windows, I think we can safely remove uvloop from dev-requirements.txt and assume it's added under Linux. --- dev-requirements.txt | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/dev-requirements.txt b/dev-requirements.txt index ac4a5d0e..27bf0221 100644 --- a/dev-requirements.txt +++ b/dev-requirements.txt @@ -34,6 +34,12 @@ click==8.2.1 # uvicorn codespell==2.4.1 # via labthings-fastapi (pyproject.toml) +colorama==0.4.6 + # via + # click + # pytest + # sphinx + # uvicorn coverage==7.9.2 # via pytest-cov dnspython==2.7.0 @@ -49,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 @@ -239,6 +249,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 @@ -249,16 +267,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 @@ -272,8 +294,6 @@ uvicorn==0.35.0 # fastapi # fastapi-cli # fastapi-cloud-cli -uvloop==0.21.0 - # via uvicorn watchfiles==1.1.0 # via uvicorn websockets==15.0.1 From 1b91ae31c31cdadb963c21ef3c9af80ea6eb4557 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 30 Jul 2025 00:06:31 +0100 Subject: [PATCH 14/59] Fix codespell config and some spelling errors Codespell should ignore docs/build and docs/src/autoapi, as both of these are generated files. --- .codespellrc | 2 ++ typing_tests/thing_definitions.py | 4 ++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/.codespellrc b/.codespellrc index a8af5d40..061e2403 100644 --- a/.codespellrc +++ b/.codespellrc @@ -7,6 +7,8 @@ skip = *.git, ./build, ./dist, ./docs/_build, + ./docs/build, + ./docs/source/autoapi, ./htmlcov, ./src/openflexure_microscope_server/static, .venv, diff --git a/typing_tests/thing_definitions.py b/typing_tests/thing_definitions.py index f75662d2..117d1ba1 100644 --- a/typing_tests/thing_definitions.py +++ b/typing_tests/thing_definitions.py @@ -65,8 +65,8 @@ class TestPropertyDefaultsMatch(lt.Thing): intprop2: int = lt.property(default="foo") # type: ignore[assignment] intprop3: int = lt.property(default_factory=optional_int_factory) # type: ignore[assignment] - # Data properies must always have a default, so this line should fail - # with mypy. It will also raise an excetion at runtime, and there's a + # Data properties must always have a default, so this line should fail + # with mypy. It will also raise an exception at runtime, and there's a # test for that run with pytest. intprop4: int = lt.property() # type: ignore[call-overload] "This property should cause mypy to throw an error, as it has no default." From 3c028654c0350a50e9e6b9667f9add37a74a72c9 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 30 Jul 2025 00:20:20 +0100 Subject: [PATCH 15/59] Fixed flake8 errors I've globally disabled some whitespace-checking as this is done better by ruff (in particular, it handles @overload better). I've disabled a few more warnings related to @overload definitions line-by-line, with a block comment explaining why at the top of the module. This is mostly flake8 not understanding @overload or @builtins.property. --- pyproject.toml | 2 + src/labthings_fastapi/thing_property.py | 52 +++++++++++-------------- 2 files changed, 25 insertions(+), 29 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 120c3ea0..41c83c4a 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -105,6 +105,8 @@ extend-ignore = [ "DOC301", # allow class + __init__ docstrings "D202", # conflicts with ruff format "F401", # already implemented by ruff, which respects "import x as x" + "E301", # leave this to ruff (blank lines) + "E302", # leave this to ruff (blank lines) "E501", # leave this to ruff (line length), #noqa may exceed max line length. ] max-line-length = 88 diff --git a/src/labthings_fastapi/thing_property.py b/src/labthings_fastapi/thing_property.py index f372bed7..3d5f96d2 100644 --- a/src/labthings_fastapi/thing_property.py +++ b/src/labthings_fastapi/thing_property.py @@ -90,10 +90,7 @@ class attribute. Documentation is in strings immediately following the # because they shouldn't have docstrings - the docstring belongs only on the # function they overload. # D105 is the same as D103, but for __init__ (i.e. magic methods). -# E302 refers to whitespace around function definitions. I have ignored this -# between @overload definitions and the function they overload, because -# it's clearer to keep them all together. Otherwise, the @overload block is easy -# to miss. +# DOC101 and DOC103 are also a result of overloads not having docstrings # DOC201 and D401 are ignored on properties. Because we are overriding the # builtin `property`, we are using `@builtins.property` which is not recognised # by pydoclint as a property. I've therefore ignored those codes manually. @@ -203,16 +200,16 @@ def default_factory() -> Value: # See comment at the top of the file regarding ignored linter rules. @overload # use as a decorator @property -def property( +def property( # noqa: D103 getter: Callable[[Any], Value], -) -> FunctionalProperty[Value]: ... # noqa: D103 +) -> FunctionalProperty[Value]: ... @overload # use as `field: int = property(0)`` def property(*, default: Value, readonly: bool = False) -> Value: ... # noqa: D103 @overload # use as `field: int = property(default_factory=lambda: 0)` # noqa: E302 def property( # noqa: D103 *, default_factory: Callable[[], Value], readonly: bool = False ) -> Value: ... -def property( # noqa: E302 +def property( getter: ValueGetter | None = None, *, default: Value | EllipsisType = ..., @@ -231,15 +228,12 @@ def property( # noqa: E302 property should either be JSON serialisable (i.e. simple built-in types) or a subclass of `pydantic.BaseModel`. - :param default: is the default value. Either this or + :param getter: is a method of a class that returns the value + of this property. This is usually supplied by using ``property`` + as a decorator. + :param default: is the default value. Either this, ``getter`` or ``default_factory`` must be specified. Specifying both or neither will raise an exception. - - When ``property`` is used as a decorator, the function - being decorated is passed as the first argument, which is - why this argument also accepts callable objects. Callable - default values are not supported. If you want to set your - default value with a function, see ``default_factory``. :param default_factory: should return your default value. This may be used as an alternative to ``default`` if you need to use a mutable datatype. For example, it would be @@ -427,11 +421,13 @@ class DataProperty(BaseProperty[Value], Generic[Value]): """ @overload - def __init__(self, default: Value, *, readonly: bool = False) -> None: ... # noqa: D105 + def __init__( # noqa: D105,D107,DOC101,DOC103 + self, default: Value, *, readonly: bool = False + ) -> None: ... @overload - def __init__( + def __init__( # noqa: D105,D107,DOC101,DOC103 self, *, default_factory: ValueFactory, readonly: bool = False - ) -> None: ... # noqa: D105 + ) -> None: ... def __init__( self, default: Value | EllipsisType = ..., @@ -784,16 +780,16 @@ def __set__(self, obj: Thing, value: Value): @overload # use as a decorator @setting -def setting( +def setting( # noqa: D103 getter: Callable[[Any], Value], -) -> FunctionalSetting[Value]: ... # noqa: D103 +) -> FunctionalSetting[Value]: ... @overload # use as `field: int = setting(0)`` def setting(*, default: Value, readonly: bool = False) -> Value: ... # noqa: D103 -@overload # use as `field: int = setting(default_factory=lambda: 0)` # noqa: E302 +@overload # use as `field: int = setting(default_factory=lambda: 0)` def setting( # noqa: D103 *, default_factory: Callable[[], Value], readonly: bool = False ) -> Value: ... -def setting( # noqa: E302 +def setting( getter: ValueGetter | None = None, *, default: Value | EllipsisType = ..., @@ -829,14 +825,12 @@ def setting( # noqa: E302 would trigger saving but ``self.dictsetting[a] = 2`` would not, as the setter for ``dictsetting`` is never called. - :param default: is the default value. Either this or - ``default_factory`` must be specified. - - When ``setting`` is used as a decorator, the function - being decorated is passed as the first argument, which is - why this argument also accepts callable objects. Callable - default values are not supported. If you want to set your - default value with a function, see ``default_factory``. + :param getter: is a method of a class that returns the value + of this property. This is usually supplied by using ``property`` + as a decorator. + :param default: is the default value. Either this, ``getter`` or + ``default_factory`` must be specified. Specifying both + or neither will raise an exception. :param default_factory: should return your default value. This may be used as an alternative to ``default`` if you need to use a mutable datatype. For example, it would be From 587829fa11f8815e5e05de2702461eeed9184035 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 30 Jul 2025 00:26:50 +0100 Subject: [PATCH 16/59] Update tests and docs to use a named `default` arg to `lt.property` --- docs/source/quickstart/counter.py | 2 +- docs/source/tutorial/properties.rst | 12 ++++++------ docs/source/tutorial/writing_a_thing.rst | 4 ++-- src/labthings_fastapi/base_descriptor.py | 4 ++-- tests/test_action_cancel.py | 4 ++-- tests/test_action_manager.py | 2 +- tests/test_properties.py | 8 ++++---- tests/test_settings.py | 4 ++-- tests/test_thing_lifecycle.py | 2 +- 9 files changed, 21 insertions(+), 21 deletions(-) diff --git a/docs/source/quickstart/counter.py b/docs/source/quickstart/counter.py index 89a455c3..efe22030 100644 --- a/docs/source/quickstart/counter.py +++ b/docs/source/quickstart/counter.py @@ -22,7 +22,7 @@ def slowly_increase_counter(self) -> None: time.sleep(1) self.increment_counter() - counter: int = lt.property(0, readonly=True) + counter: int = lt.property(default=0, readonly=True) "A pointless counter" diff --git a/docs/source/tutorial/properties.rst b/docs/source/tutorial/properties.rst index 0ce85742..3ec536be 100644 --- a/docs/source/tutorial/properties.rst +++ b/docs/source/tutorial/properties.rst @@ -15,14 +15,14 @@ Data properties behave like variables: they simply store a value that is used by import labthings_fastapi as lt class MyThing(lt.Thing): - my_property: int = lt.property(42) + my_property: int = lt.property(default=42) The example above defines a property called `my_property` that has a default value of `42`. Note the type hint `int` which indicates that the property should hold an integer value. This is important, as the type will be enforced when the property is written to via HTTP, and it will appear in :ref:`gen_docs`. By default, this property may be read or written to by HTTP requests. If you want to make it read-only, you can set the `readonly` parameter to `True`: .. code-block:: python class MyThing(lt.Thing): - my_property: int = lt.property(42, readonly=True) + my_property: int = lt.property(default=42, readonly=True) Note that the ``readonly`` parameter only affects *client* code, i.e. it may not be written to via HTTP requests or `.DirectThingClient` instances. However, the property can still be modified by the Thing's code, e.g. in response to an action or another property change as ``self.my_property = 100``. @@ -31,7 +31,7 @@ It is a good idea to make sure there is a docstring for your property. This will .. code-block:: python class MyThing(lt.Thing): - my_property: int = lt.property(42, readonly=True) + my_property: int = lt.property(default=42, readonly=True) """A property that holds an integer value.""" You don't need to include the type in the docstring, as it will be inferred from the type hint. However, you can include additional information about the property, such as its units or any constraints on its value. @@ -48,7 +48,7 @@ It is also possible to have properties that run code when they are read or writt import labthings_fastapi as lt class MyThing(lt.Thing): - my_property: int = lt.property(42) + my_property: int = lt.property(default=42) """A property that holds an integer value.""" @lt.property @@ -65,7 +65,7 @@ Functional properties may also have a "setter" method, which is called when the import labthings_fastapi as lt class MyThing(lt.Thing): - my_property: int = lt.property(42) + my_property: int = lt.property(default=42) """A property that holds an integer value.""" @lt.property @@ -85,7 +85,7 @@ Adding a setter makes the property read-write (if only a getter is present, it m import labthings_fastapi as lt class MyThing(lt.Thing): - my_property: int = lt.property(42) + my_property: int = lt.property(default=42) """A property that holds an integer value.""" @lt.property diff --git a/docs/source/tutorial/writing_a_thing.rst b/docs/source/tutorial/writing_a_thing.rst index d8ad37b4..06d9c134 100644 --- a/docs/source/tutorial/writing_a_thing.rst +++ b/docs/source/tutorial/writing_a_thing.rst @@ -16,8 +16,8 @@ Our first Thing will pretend to be a light: we can set its brightness and turn i import labthings_fastapi as lt class Light(lt.Thing): - brightness: int = lt.property(100) - is_on: bool = lt.property(False, readonly=true) + brightness: int = lt.property(default=100) + is_on: bool = lt.property(default=False, readonly=true) @lt.action def toggle(self): diff --git a/src/labthings_fastapi/base_descriptor.py b/src/labthings_fastapi/base_descriptor.py index eb0a8f77..37064b26 100644 --- a/src/labthings_fastapi/base_descriptor.py +++ b/src/labthings_fastapi/base_descriptor.py @@ -34,10 +34,10 @@ class DescriptorNotAddedToClassError(RuntimeError): class Test(lt.Thing): - myprop: int = lt.property(0) # This is OK + myprop: int = lt.property(default=0) # This is OK - orphaned_prop: int = lt.property(0) # Not OK + orphaned_prop: int = lt.property(default=0) # Not OK Test.myprop.model # Evaluates to a pydantic model diff --git a/tests/test_action_cancel.py b/tests/test_action_cancel.py index 03ee939f..ead108e7 100644 --- a/tests/test_action_cancel.py +++ b/tests/test_action_cancel.py @@ -10,8 +10,8 @@ class CancellableCountingThing(lt.Thing): - counter: int = lt.property(0) - check: bool = lt.property(False) + counter: int = lt.property(default=0) + check: bool = lt.property(default=False) """Whether the count has been cancelled. This variable is used to check that the action can detect a cancel event diff --git a/tests/test_action_manager.py b/tests/test_action_manager.py index 606d1be4..c38b50d7 100644 --- a/tests/test_action_manager.py +++ b/tests/test_action_manager.py @@ -13,7 +13,7 @@ def increment_counter(self): """Increment the counter""" self.counter += 1 - counter: int = lt.property(0, readonly=True) + counter: int = lt.property(default=0, readonly=True) "A pointless counter" diff --git a/tests/test_properties.py b/tests/test_properties.py index a5f9e5d1..c660df09 100644 --- a/tests/test_properties.py +++ b/tests/test_properties.py @@ -9,10 +9,10 @@ class TestThing(lt.Thing): - boolprop: bool = lt.property(False) + boolprop: bool = lt.property(default=False) "A boolean property" - stringprop: str = lt.property("foo") + stringprop: str = lt.property(default="foo") "A string property" _undoc = None @@ -56,7 +56,7 @@ def test_instantiation_with_type(): # This will not work unless the property is assigned to a thing class TestThing(lt.Thing): - prop: bool = lt.property(False) + prop: bool = lt.property(default=False) assert issubclass(TestThing.prop.model, BaseModel) @@ -67,7 +67,7 @@ class MyModel(BaseModel): b: float = 2.0 class Dummy: - prop: MyModel = lt.property(MyModel()) + prop: MyModel = lt.property(default=MyModel()) @lt.property def func_prop(self) -> MyModel: diff --git a/tests/test_settings.py b/tests/test_settings.py index f7cd8a45..907d75a3 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -18,10 +18,10 @@ def __init__(self) -> None: # Initialize functional settings with default values self._floatsetting: float = 1.0 - boolsetting: bool = lt.setting(False) + boolsetting: bool = lt.setting(default=False) "A boolean setting" - stringsetting: str = lt.setting("foo") + stringsetting: str = lt.setting(default="foo") "A string setting" dictsetting: dict = lt.setting(default_factory=lambda: {"a": 1, "b": 2}) diff --git a/tests/test_thing_lifecycle.py b/tests/test_thing_lifecycle.py index 86062bce..1024cd5c 100644 --- a/tests/test_thing_lifecycle.py +++ b/tests/test_thing_lifecycle.py @@ -3,7 +3,7 @@ class TestThing(lt.Thing): - alive: bool = lt.property(False) + alive: bool = lt.property(default=False) "Whether the thing is alive." def __enter__(self): From 029343f11d9b328fff39c669e53d4811f76d389d Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 30 Jul 2025 00:36:45 +0100 Subject: [PATCH 17/59] More helpful error if a non-callable value is passed as the positional arg --- src/labthings_fastapi/thing_property.py | 28 +++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/src/labthings_fastapi/thing_property.py b/src/labthings_fastapi/thing_property.py index 3d5f96d2..2fa9ac63 100644 --- a/src/labthings_fastapi/thing_property.py +++ b/src/labthings_fastapi/thing_property.py @@ -210,7 +210,7 @@ def property( # noqa: D103 *, default_factory: Callable[[], Value], readonly: bool = False ) -> Value: ... def property( - getter: ValueGetter | None = None, + getter: ValueGetter | EllipsisType = ..., *, default: Value | EllipsisType = ..., default_factory: ValueFactory | None = None, @@ -250,6 +250,9 @@ def property( if used as a decorator, or a `.DataProperty` if used as a field. + :raises MissingDefaultError: if no valid default value is supplied, + and a getter is not in use. + **Typing Notes** This function has somewhat complicated type hints, for two reasons. @@ -275,9 +278,15 @@ def property( distinguish between ``default`` not being set (``...``) and a desired default value of ``None``. """ - if callable(getter): + if getter is not ...: # If the default is callable, we're being used as a decorator # without arguments. + if not callable(getter): + raise MissingDefaultError( + "A non-callable getter was passed to `property`. Usually," + "this means the default value was not passed as a keyword " + "argument, which is required." + ) return FunctionalProperty( fget=getter, ) @@ -790,7 +799,7 @@ def setting( # noqa: D103 *, default_factory: Callable[[], Value], readonly: bool = False ) -> Value: ... def setting( - getter: ValueGetter | None = None, + getter: ValueGetter | EllipsisType = ..., *, default: Value | EllipsisType = ..., default_factory: ValueFactory | None = None, @@ -843,17 +852,24 @@ def setting( :return: a setting descriptor. + :raises MissingDefaultError: if no valid default is supplied. + **Typing Notes** The return type of this function is a "white lie" in order to allow dataclass-style type annotations """ - if callable(default): + if getter is not ...: # If the default is callable, we're being used as a decorator # without arguments. - func = default + if not callable(getter): + raise MissingDefaultError( + "A non-callable getter was passed to `property`. Usually," + "this means the default value was not passed as a keyword " + "argument, which is required." + ) return FunctionalSetting( - fget=func, + fget=getter, ) return DataSetting( # type: ignore[return-value] default_factory=default_factory_from_arguments(default, default_factory), From 14d3dd9aee93f27f4a465fd708f6dc6b39388801 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 30 Jul 2025 00:36:51 +0100 Subject: [PATCH 18/59] Add to external linter codes to stop ruff warning about unrecognised codes. --- pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index 41c83c4a..29bfe47c 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -76,7 +76,7 @@ target-version = "py310" docstring-code-format = true [tool.ruff.lint] -external = ["DOC401", "F824"] # used via flake8/pydoclint +external = ["DOC401", "F824", "DOC101", "DOC103"] # used via flake8/pydoclint select = ["E4", "E7", "E9", "F", "D", "DOC"] ignore = [ "D203", # incompatible with D204 From 551e3085e9f25e222c7903f277bdfa2744938c95 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 30 Jul 2025 09:16:20 +0100 Subject: [PATCH 19/59] Make CI compatible with Python 3.10 and Linux There was a windows path with a backslash accidentally in the CI file. I also needed to switch a few imports to `typing_extensions`. This trips me up sufficiently often that it's tempting to drop Python 3.10. --- .github/workflows/test.yml | 2 +- src/labthings_fastapi/base_descriptor.py | 3 ++- src/labthings_fastapi/thing_property.py | 2 +- typing_tests/thing_definitions.py | 2 +- 4 files changed, 5 insertions(+), 4 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 958d1f3c..c39aa30c 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -77,7 +77,7 @@ jobs: run: mypy src - name: Type tests with MyPy - run: mypy --warn-unused-ignores .\typing_tests\ + run: mypy --warn-unused-ignores ./typing_tests/ test-with-unpinned-deps: runs-on: ubuntu-latest diff --git a/src/labthings_fastapi/base_descriptor.py b/src/labthings_fastapi/base_descriptor.py index 37064b26..d06e633c 100644 --- a/src/labthings_fastapi/base_descriptor.py +++ b/src/labthings_fastapi/base_descriptor.py @@ -6,7 +6,8 @@ """ from __future__ import annotations -from typing import overload, Generic, Self, TypeVar, TYPE_CHECKING +from typing import overload, Generic, TypeVar, TYPE_CHECKING +from typing_extensions import Self from .utilities.introspection import get_summary diff --git a/src/labthings_fastapi/thing_property.py b/src/labthings_fastapi/thing_property.py index 2fa9ac63..2b4779bc 100644 --- a/src/labthings_fastapi/thing_property.py +++ b/src/labthings_fastapi/thing_property.py @@ -52,12 +52,12 @@ class attribute. Documentation is in strings immediately following the Any, Callable, Generic, - Self, TypeAlias, TypeVar, overload, TYPE_CHECKING, ) +from typing_extensions import Self import typing from weakref import WeakSet diff --git a/typing_tests/thing_definitions.py b/typing_tests/thing_definitions.py index 117d1ba1..3c54f476 100644 --- a/typing_tests/thing_definitions.py +++ b/typing_tests/thing_definitions.py @@ -9,7 +9,7 @@ import labthings_fastapi as lt from labthings_fastapi.thing_property import FunctionalProperty -from typing import assert_type +from typing_extensions import assert_type import typing From 1d37e8078afe1d3ab4bdad336b0f00aa8bef6625 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 30 Jul 2025 10:42:52 +0100 Subject: [PATCH 20/59] Implement, and test, docstring-after-the-property syntax. This commit adds functionality to `BaseDescriptor` that pulls docstrings from the owning class definition. See the `BaseDescriptor` docstring for details. I've added unit tests for all properties of BaseDescriptor, and for the internal method get_class_attribute_docstrings, including its cache. --- src/labthings_fastapi/base_descriptor.py | 146 ++++++++++++++++++++++- tests/test_base_descriptor.py | 76 ++++++++++++ 2 files changed, 218 insertions(+), 4 deletions(-) create mode 100644 tests/test_base_descriptor.py diff --git a/src/labthings_fastapi/base_descriptor.py b/src/labthings_fastapi/base_descriptor.py index d06e633c..5e9049d3 100644 --- a/src/labthings_fastapi/base_descriptor.py +++ b/src/labthings_fastapi/base_descriptor.py @@ -6,10 +6,16 @@ """ from __future__ import annotations -from typing import overload, Generic, TypeVar, TYPE_CHECKING +import ast +import inspect +from itertools import pairwise +import textwrap +from typing import overload, Generic, Mapping, TypeVar, TYPE_CHECKING +from types import MappingProxyType +from weakref import WeakKeyDictionary from typing_extensions import Self -from .utilities.introspection import get_summary +from .utilities.introspection import get_docstring, get_summary if TYPE_CHECKING: from .thing import Thing @@ -53,8 +59,29 @@ class BaseDescriptor(Generic[Value]): * The descriptor remembers the name it's assigned to in ``name``, for use in :ref:`gen_docs`\ . + * The descriptor inspects its owning class, and looks for an attribute + docstring (i.e. a string constant immediately following the attribute + assignment). * When called as a class attribute, the descriptor returns itself, as done by e.g. `property`. + * The docstring and name are used to provide a ``title`` and ``description`` + that may be used in :ref:`gen_docs` and elsewhere. + + .. code-block:: python + + class Example: + my_prop = BaseDescriptor() + '''My Property. + + This is a nice long docstring describing my property, which + can span multiple lines. + ''' + + + p = Example.my_prop + assert p.name == "my_prop" + assert p.title == "My Property." + assert p.description.startswith("This is") """ def __init__(self) -> None: @@ -73,8 +100,13 @@ def __set_name__(self, owner: type[Thing], name: str) -> None: r"""Take note of the name to which the descriptor is assigned. This is called when the descriptor is assigned to an attribute of a class. - This function just remembers the name, so it can be used in - :ref:`gen_docs`\ . + This function remembers the name, so it can be used in :ref:`gen_docs`\ . + + This function also inspects the owning class, and will retrieve the + docstring for its attribute. This allows us to use a string immediately + after the descriptor is defined, rather than passing the docstring as + an argument. + See `.get_class_attribute_docstrings` for more details. :param owner: the `.Thing` subclass to which we are being attached. :param name: the name to which we have been assigned. @@ -84,6 +116,12 @@ def __set_name__(self, owner: type[Thing], name: str) -> None: self._name = name self._set_name_called = True + # Check for docstrings on the owning class, and retrieve the one for + # this attribute (identified by `name`). + attr_docs = get_class_attribute_docstrings(owner) + if name in attr_docs: + self.__doc__ = attr_docs[name] + def assert_set_name_called(self): """Raise an exception if ``__set_name__`` has not yet been called. @@ -160,6 +198,7 @@ def description(self) -> str | None: As the first line of the docstring (if present) is used as the ``title`` in :ref:`gen_docs` it will be removed from this property. """ + return get_docstring(self, remove_summary=True) # I have ignored D105 (missing docstrings) on the overloads - these should not # exist on @overload definitions. @@ -213,3 +252,102 @@ def instance_get(self, obj: Thing) -> Value: "__instance_get__ must be defined on BaseDescriptor subclasses. \n\n" "See BaseDescriptor.__instance_get__ for details." ) + + +_class_attribute_docstring_cache: WeakKeyDictionary[type, Mapping[str, str]] = ( + WeakKeyDictionary() +) + + +def get_class_attribute_docstrings(cls: type) -> Mapping[str, str]: + """Retrieve docstrings for the attributes of a class. + + Python formally supports ``__doc__`` attributes on classes and functions, and + this means that classes and methods can self-describe in a way that is picked + up by documentation tools. There isn't currently a language feature specifically + provided to annotate other attributes of a class, but there is a convention + that seems almost universally adopted by documentation tools, which is to + add a string literal immediately after the attribute assignment. While it's + not a formal language feature, Python does explicitly allow these string + literals (which don't have any other purpose) to enable documentation tools + to document attributes. + + This function inspects a class, and returns a dictionary mapping attribute + names to docstrings, where the docstring is a string immediately following + the attribute. For example: + + .. code-block:: python + + class Example: + my_constant: int = 10 + "A number that is all mine." + + + docs = get_class_attribute_docstrings(Example) + + assert docs["my_constant"] == "A number that is all mine." + + + .. note:: + + This function relies on re-parsing the source of the class, so it will + not work on classes that are not defined in a file (for example, if you + just paste the example above into a Python interpreter). In that case, + an empty dictionary is returned. + + :param cls: The class to inspect + :return: A mapping of attribute names to docstrings. Note that this will be + wrapped in a `types.MappingProxyType` to prevent accidental modification. + """ + # For a helpful article on how this works, see: + # https://davidism.com/attribute-docstrings/ + if cls in _class_attribute_docstring_cache: # Attempt to use the cache + return _class_attribute_docstring_cache[cls] + + # We start by getting hold of the source code of our class. This requires + # the class to be loaded from a file, which is nearly always the case. + # We will simply return an empty dictionary if this fails: there is never + # any guarantee docstrings are available. + try: + src = inspect.getsource(cls) + except OSError: + # An OSError is raised if the source is not available. + return {} + # The line below parses the class to get a syntax tree. + module_ast = ast.parse(textwrap.dedent(src)) + assert isinstance(module_ast, ast.Module) + class_def = module_ast.body[0] + if not isinstance(class_def, ast.ClassDef): + raise TypeError("The object supplied was not a class.") + # Work through each pair of nodes, looking for an assignment followed by + # a string. + docs: dict[str, str] = {} + for a, b in pairwise(class_def.body): + if not isinstance(a, ast.Assign | ast.AnnAssign): + continue # The first node isn't an assignment + if ( + not isinstance(b, ast.Expr) + or not isinstance(b.value, ast.Constant) + or not isinstance(b.value.value, str) + ): + continue # The second node must be a string constant + + # Assignments may have multiple targets (a=b=c) so we + # need to cope with a list of targets. + if isinstance(a, ast.Assign): + targets = a.targets + else: # Annotated asignments have only one target, so make it a list. + targets = [a.target] + + # Clean up the docstring as per the usual rules + doc = inspect.cleandoc(b.value.value) + + for target in targets: + if not isinstance(target, ast.Name): + # We only care about things assigned to plain names. Assignment to + # attributes of objects, or items in dictionaries, are irrelevant. + continue + docs[target.id] = doc + + _class_attribute_docstring_cache[cls] = MappingProxyType(docs) + return _class_attribute_docstring_cache[cls] diff --git a/tests/test_base_descriptor.py b/tests/test_base_descriptor.py new file mode 100644 index 00000000..9e00e60b --- /dev/null +++ b/tests/test_base_descriptor.py @@ -0,0 +1,76 @@ +import pytest +from labthings_fastapi.base_descriptor import ( + BaseDescriptor, + DescriptorNotAddedToClassError, + get_class_attribute_docstrings, +) + + +class MockProperty(BaseDescriptor[str]): + """A mock property class.""" + + # The line below isn't defined on a `Thing`, so mypy + # errors - but we ignore this for testing. + def instance_get(obj) -> str: # type: ignore[override] + return "An example value." + + +class Example: + my_constant: int = 10 + "A number that is all mine." + + my_undocumented_constant: int = 20 + + my_property = MockProperty() + "Docs for my_property." + + my_undocumented_property = MockProperty() + + my_property_with_nice_docs = MockProperty() + """Title goes here. + + The docstring should have a one-line title followed by + a body giving a longer description of what's going on. + """ + + +def test_docstrings_are_retrieved(): + """Check that the docstring can be picked up from the class definition.""" + docs = get_class_attribute_docstrings(Example) + assert docs["my_constant"] == "A number that is all mine." + assert docs["my_property"] == "Docs for my_property." + + +def test_docstrings_are_cached(): + """Check that the docstrings aren't being regenerated every time.""" + docs1 = get_class_attribute_docstrings(Example) + docs2 = get_class_attribute_docstrings(Example) + assert docs1 is docs2 + + +def test_basedescriptor_with_good_docstring(): + """Check we get the right documentation properties.""" + prop = Example.my_property_with_nice_docs + assert prop.name == "my_property_with_nice_docs" + assert prop.title == "Title goes here." + assert prop.description.startswith("The docstring") + + +def test_basedescriptor_orphaned(): + """Check the right error is raised if we ask for the name outside a class.""" + prop = MockProperty() + with pytest.raises(DescriptorNotAddedToClassError): + prop.name + + +def test_basedescriptor_fallback(): + """Check the title defaults to the name.""" + p = Example.my_undocumented_property + assert p.title == "my_undocumented_property" + assert p.__doc__ is None + assert p.description is None + + +def test_basedescriptor_get(): + e = Example() + assert e.my_property == "An example value." From 6927ce362fe1cfee18480d6c477ff8f81eb56391 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 30 Jul 2025 14:19:37 +0100 Subject: [PATCH 21/59] Documentation improvements from review and codespell --- docs/source/documentation.rst | 21 +++++++++++++-- docs/source/see_also.rst | 33 +++++++++++++++++++++--- docs/source/tutorial/properties.rst | 31 +++++++++++++++++++--- src/labthings_fastapi/base_descriptor.py | 2 +- 4 files changed, 78 insertions(+), 9 deletions(-) diff --git a/docs/source/documentation.rst b/docs/source/documentation.rst index 6ea4a322..5a781435 100644 --- a/docs/source/documentation.rst +++ b/docs/source/documentation.rst @@ -5,11 +5,28 @@ Generated documentation LabThings describes its HTTP API in two ways: with a :ref:`wot_td` and with an OpenAPI_ document. +.. _openapi: + +OpenAPI +------- + OpenAPI_ is a standard way to describe an HTTP interface. It lists all of the possible HTTP requests that may be made, along with a description of each one, and a description of the possible responses. -Each :ref:`wot_thing` is documented by a Thing Description, which is a JSON document describing all of the ways to interact with that Thing. The WoT_ standard defines the `Thing Description`_ and includes a JSON Schema against which it may be validated. +.. _gen_td: + +Thing Description +----------------- + +Each :ref:`wot_thing` is documented by a Thing Description, which is a JSON document describing all of the ways to interact with that Thing (:ref:`wot_affordances`\ ). The WoT_ standard defines the `Thing Description`_ and includes a JSON Schema against which it may be validated. + +Thing Description documents are higher-level than OpenAPI_ and focus on the capabilities of the Thing. For example, they include a list of properties, where each action is described only once. LabThings treats the Thing Description as your public API, and as a general rule anything not described in the Thing Description is not available over HTTP or to a `.DirectThingClient`\ . + +Comparison of Thing Description and OpenAPI +------------------------------------------- + +Thing Description aims to be a neat way to describe the capabilities of a Thing, while OpenAPI focuses on detailed documentation of every possible interaction with the server. Thing Description is a newer and less well adopted standard that's specific to the Web of Things, while OpenAPI has been around for a while and is widely used and understood as a general-purpose API description. -Thing Description documents are higher-level than OpenAPI_ and focus on the capabilities of the Thing, rather than the HTTP endpoints. In general you would expect more HTTP endpoints than there are interaction affordances, so in principle client code based on a Thing Description should be more meaningful. However, OpenAPI is a much more widely adopted standard and so both forms of documentation are generated by LabThings-FastAPI. +OpenAPI describes each HTTP endpoint individually. There are usually more HTTP endpoints than there are :ref:`wot_affordances` because a Property may have two endpoints, one to read its value and one to write it. Actions usually correspond to several endpoints; one to invoke the action, one to check the action's status, one to cancel it, and another to retrieve its output once it has finished. In principle, client code based on a Thing Description should be more meaningful because related endpoints can be grouped together into properties or actions that correspond to structures in the programming language (like methods and properties in Python). However, OpenAPI is a much more widely adopted standard and so both forms of documentation are generated by LabThings-FastAPI. .. _WoT: https://www.w3.org/WoT/ .. _Thing Description: https://www.w3.org/TR/wot-thing-description/ diff --git a/docs/source/see_also.rst b/docs/source/see_also.rst index dc5781c5..8fe800e3 100644 --- a/docs/source/see_also.rst +++ b/docs/source/see_also.rst @@ -8,14 +8,41 @@ LabThings-FastAPI makes quite heavy use of a few key concepts from external libr Descriptors ----------- -Descriptors are a way to itercept attribute access on an object. By default, attributes of an object are just variables - so an object called ``foo`` might have an attribute called ``bar``, and you may read its value with ``foo.bar``, write its value with ``foo.bar = "baz"``, and delete the attribute with ``del foo.bar``. If ``foo`` is a descriptor, Python will call the ``__get__`` method of that descriptor when it's read and the ``__set__`` method when it's written to. The descriptor protocol is described with plenty of examples in the `Descriptor Guide`_ in the Python documentation. +Descriptors are a way to itercept attribute access on an object. By default, attributes of an object are just variables - so an object called ``foo`` might have an attribute called ``bar``, and you may read its value with ``foo.bar``, write its value with ``foo.bar = "baz"``, and delete the attribute with ``del foo.bar``. If ``foo`` is a descriptor, Python will call the ``__get__`` method of that descriptor when it's read and the ``__set__`` method when it's written to. You have quite probably used a descriptor already, because the built-in `~builtins.property` creates a descriptor object: that's what runs your getter method when the property is accessed. The descriptor protocol is described with plenty of examples in the `Descriptor Guide`_ in the Python documentation. In LabThings-FastAPI, descriptors are used to implement :ref:`wot_actions` and :ref:`wot_properties` on `.Thing` subclasses. The intention is that these will function like standard Python methods and properties, but will also be available over HTTP, along with automatic documentation in the :ref:`wot_td` and OpenAPI documents. There are a few useful notes that relate to many of the descriptors in LabThings-FastAPI: -* Descriptor objects **may have more than one owner**. As a rule, a descriptor object (e.g. an instance of `.DataProperty`) is assigned to an attribute of one `.Thing` subclass. There may, however, be multiple *instances* of that class, so it is not safe to assume that the descriptor object corresponds to only one `.Thing`. This is why the `.Thing` is passed to the ``__get__`` method: we should ensure that any values being remembered are keyed to the owning `.Thing` and are not simply stored in the descriptor. Usually, this is done using `.WeakKeyDictionary` objects, which allow us to look up values based on the `.Thing`, without interfering with garbage collection. +* Descriptor objects **may have more than one owner**. As a rule, a descriptor object + (e.g. an instance of `.DataProperty`) is assigned to an attribute of one `.Thing` subclass. There may, however, be multiple *instances* of that class, so it is not safe to assume that the descriptor object corresponds to only one `.Thing`. This is why the `.Thing` is passed to the ``__get__`` method: we should ensure that any values being remembered are keyed to the owning `.Thing` and are not simply stored in the descriptor. Usually, this is done using `.WeakKeyDictionary` objects, which allow us to look up values based on the `.Thing`, without interfering with garbage collection. + + The example below shows how this can go wrong. + + .. code-block:: python + + class BadProperty: + "An example of a descriptor that has unwanted behaviour." + def __init__(self): + self._value = None + + def __get__(self, obj): + return self._value + + def __set__(self, obj, val): + self._value = val + + class BrokenExample: + myprop = BadProperty() + + a = BrokenExample() + b = BrokenExample() + + assert a.myprop is None + b.myprop = True + assert a.myprop is None # FAILS because `myprop` shares values between a and b + * Descriptor objects **may know their name**. Python calls ``__set_name__`` on a descriptor if it is available. This allows the descriptor to know the name of the attribute to which it is assigned. LabThings-FastAPI uses the name in the URL and in the Thing Description. When ``__set_name__`` is called, the descriptor **is also passed the class that owns it**. This allows us to check for type hints and docstrings that are part of the class, rather than part of the descriptor. -* There is a convention that descriptors return their value when accessed as an instance attribute, but return themselves when accessed as a class attribute. We try to adhere to that convention. +* There is a convention that descriptors return their value when accessed as an instance attribute, but return themselves when accessed as a class attribute (as done by `builtins.property`). LabThings adheres to that convention. .. _`Descriptor Guide`: https://docs.python.org/3/howto/descriptor.html \ No newline at end of file diff --git a/docs/source/tutorial/properties.rst b/docs/source/tutorial/properties.rst index 3ec536be..9e3b970e 100644 --- a/docs/source/tutorial/properties.rst +++ b/docs/source/tutorial/properties.rst @@ -3,7 +3,16 @@ Properties ========================= -:ref:`wot_properties` are values that can be read and written on a Thing. They are used to represent the state of the Thing, such as its current temperature, brightness, or status. Properties can be read using a ``GET`` request and written using a ``PUT`` or ``POST`` request. You can add properties to a `.Thing` by using `.property` (usually imported as ``lt.property``). + + +Properties are values that can be read from and written to a Thing. They are used to represent the state of the Thing, such as its current temperature, brightness, or status. :ref:`wot_properties` are a key concept in the Web of Things standard. + +LabThings implements properties in a very similar way to the built-in Python `~builtins.property`. The key difference is that defining an attribute as a `.property` means that the property will be listed in the :ref:`gen_td` and exposed over HTTP. This is important for two reasons: + +* Only properties declared using `.property` (usually imported as ``lt.property``) can be accessed over HTTP. Regular attributes or properties using `builtins.property` are only available to your `.Thing` internally, except in some special cases. +* Communication between `.Thing`\ s within a LabThings server should be done using a `.DirectThingClient` class. The purpose of `.DirectThingClient` is to provide the same interface as a `.ThingClient` over HTTP, so it will also only expose functionality described in the Thing Description. + +You can add properties to a `.Thing` by using `.property` (usually imported as ``lt.property``). Data properties ------------------------- @@ -78,7 +87,9 @@ Functional properties may also have a "setter" method, which is called when the """Set the value of twice_my_property.""" self.my_property = value // 2 -Adding a setter makes the property read-write (if only a getter is present, it must be read-only). It is possible to make a property read-only for clients by setting its ``readonly`` attribute: this has the same behaviour as for data properties, i.e. it prevents the property from being written to via HTTP requests or `.DirectThingClient` instances, but it can still be modified by the Thing's code. +Adding a setter makes the property read-write (if only a getter is present, it must be read-only). + +It is possible to make a property read-only for clients by setting its ``readonly`` attribute: this has the same behaviour as for data properties. .. code-block:: python @@ -101,7 +112,21 @@ Adding a setter makes the property read-write (if only a getter is present, it m # Make the property read-only for clients twice_my_property.readonly = True -Functional properties may not be observed, as they are not backed by a simple value. If you need to notify clients when the value changes, you can use a data property that is updated by the functional property. +In the example above, ``twice_my_property`` may be set by code within ``MyThing`` but cannot be written to via HTTP requests or `.DirectThingClient` instances. + +Functional properties may not be observed, as they are not backed by a simple value. If you need to notify clients when the value changes, you can use a data property that is updated by the functional property. In the example above, ``my_property`` may be observed, while ``twice_my_property`` cannot be observed. It would be possible to observe changes in ``my_property`` and then query ``twice_my_property`` for its new value. + +HTTP interface +-------------- + +LabThings is primarily controlled using HTTP. Mozilla have a good `Overview of HTTP`_ that is worth a read if you are unfamiliar with the concept of requests, or what ``GET`` and ``PUT`` mean. + +Each property in LabThings will be assigned a URL, which allows it to be read and (optionally) written to. The easiest way to explore this is in the interactive OpenAPI documentation, served by your LabThings server at ``/docs``\ . Properties can be read using a ``GET`` request and written using a ``PUT`` request. + +LabThings follows the `HTTP Protocol Binding`_ from the Web of Things standard. That's quite a detailed document: for a gentle introduction to HTTP and what a request means, see + +.. _`Overview of HTTP`: https://developer.mozilla.org/en-US/docs/Web/HTTP/Guides/Overview +.. _`HTTP Protocol Binding`: https://w3c.github.io/wot-binding-templates/bindings/protocols/http/index.html Observable properties ------------------------- diff --git a/src/labthings_fastapi/base_descriptor.py b/src/labthings_fastapi/base_descriptor.py index 5e9049d3..64a40a3a 100644 --- a/src/labthings_fastapi/base_descriptor.py +++ b/src/labthings_fastapi/base_descriptor.py @@ -336,7 +336,7 @@ class Example: # need to cope with a list of targets. if isinstance(a, ast.Assign): targets = a.targets - else: # Annotated asignments have only one target, so make it a list. + else: # Annotated assignments have only one target, so make it a list. targets = [a.target] # Clean up the docstring as per the usual rules From 315725b07f2020791800dbe66b81d9be0c0c3fd6 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 30 Jul 2025 22:09:38 +0100 Subject: [PATCH 22/59] Fix test of BaseDescriptor and docs `test_docs.py` was failing because of an incompatibility between `WindowsPath` and `inspect.getsource`. I've fixed this by converting the path to a string, and also handled the exception in get_class_attribute_docstrings (by returning an empty dict). I think this is reasonable: it's not an error that can be recovered from, and there is never any guarantee that docstrings are not `None`. The BaseDescriptor test was failing simply because I defined a mock function incorrectly - now fixed. --- src/labthings_fastapi/base_descriptor.py | 6 +++++- tests/test_base_descriptor.py | 2 +- tests/test_docs.py | 5 ++++- 3 files changed, 10 insertions(+), 3 deletions(-) diff --git a/src/labthings_fastapi/base_descriptor.py b/src/labthings_fastapi/base_descriptor.py index 64a40a3a..be558a23 100644 --- a/src/labthings_fastapi/base_descriptor.py +++ b/src/labthings_fastapi/base_descriptor.py @@ -298,6 +298,8 @@ class Example: :param cls: The class to inspect :return: A mapping of attribute names to docstrings. Note that this will be wrapped in a `types.MappingProxyType` to prevent accidental modification. + + :raises TypeError: if the supplied object is not a class. """ # For a helpful article on how this works, see: # https://davidism.com/attribute-docstrings/ @@ -310,8 +312,10 @@ class Example: # any guarantee docstrings are available. try: src = inspect.getsource(cls) - except OSError: + except (OSError, AttributeError): # An OSError is raised if the source is not available. + # An AttributeError is raised if the source was loaded from + # a WindowsPath object, perhaps using ``runpy`` return {} # The line below parses the class to get a syntax tree. module_ast = ast.parse(textwrap.dedent(src)) diff --git a/tests/test_base_descriptor.py b/tests/test_base_descriptor.py index 9e00e60b..aeaf032b 100644 --- a/tests/test_base_descriptor.py +++ b/tests/test_base_descriptor.py @@ -11,7 +11,7 @@ class MockProperty(BaseDescriptor[str]): # The line below isn't defined on a `Thing`, so mypy # errors - but we ignore this for testing. - def instance_get(obj) -> str: # type: ignore[override] + def instance_get(self, _obj) -> str: # type: ignore[override] return "An example value." diff --git a/tests/test_docs.py b/tests/test_docs.py index 3e9fcdfb..954b1068 100644 --- a/tests/test_docs.py +++ b/tests/test_docs.py @@ -12,7 +12,10 @@ def run_quickstart_counter(): # A server is started in the `__name__ == "__main__" block` - run_path(docs / "quickstart" / "counter.py") + # Running from a WindowsPath confuses the documentation code + # in `base_descriptor.get_class_attribute_docstrings` hence + # the cast to a `str` + run_path(str(docs / "quickstart" / "counter.py")) def test_quickstart_counter(): From efc330eb033cac7b2010d7f0a2b860a0437a6a79 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 30 Jul 2025 22:14:29 +0100 Subject: [PATCH 23/59] Fix typing tests foder in CI --- .github/workflows/test.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index c39aa30c..21d449cf 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -77,7 +77,7 @@ jobs: run: mypy src - name: Type tests with MyPy - run: mypy --warn-unused-ignores ./typing_tests/ + run: mypy --warn-unused-ignores typing_tests test-with-unpinned-deps: runs-on: ubuntu-latest From 531e87e2e8bdd4fdcf8c41722997f46bc8b55e9f Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 30 Jul 2025 22:24:02 +0100 Subject: [PATCH 24/59] Comments on modified settings test --- tests/test_settings.py | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/tests/test_settings.py b/tests/test_settings.py index 907d75a3..17b55477 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -111,10 +111,16 @@ def test_functional_settings_save(thing, server): # No setting file created when first added assert not os.path.isfile(setting_file) with TestClient(server.app) as client: + # We write a new value to the property with a PUT request r = client.put("/thing/floatsetting", json=2.0) + # A 201 return code means the operation succeeded (i.e. + # the property was written to) assert r.status_code == 201 + # We check the value with a GET request r = client.get("/thing/floatsetting") assert r.json() == 2.0 + # After successfully writing to the setting, it should + # have created a settings file. assert os.path.isfile(setting_file) with open(setting_file, "r", encoding="utf-8") as file_obj: # Check settings on file match expected dictionary @@ -128,13 +134,19 @@ def test_data_settings_save(thing, server): a different code path to the functional setting above.""" setting_file = _get_setting_file(server, "/thing") server.add_thing(thing, "/thing") - # No setting file created when first added + # The settings file should not be created yet - it's created the + # first time we write to a setting. assert not os.path.isfile(setting_file) with TestClient(server.app) as client: + # Change the value using a PUT request r = client.put("/thing/boolsetting", json=True) + # Check the value was written successfully (201 response code) assert r.status_code == 201 + # Check the value is what we expect r = client.get("/thing/boolsetting") assert r.json() is True + # After successfully writing to the setting, it should + # have created a settings file. assert os.path.isfile(setting_file) with open(setting_file, "r", encoding="utf-8") as file_obj: # Check settings on file match expected dictionary From ab54ce779b335ca518d3f97f3c5f847f577b5ddf Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 30 Jul 2025 22:34:01 +0100 Subject: [PATCH 25/59] Add a comment, and test both object and class attributes Thanks @bprobert97 for spotting that. I added an explicit check that the descriptor is returned when accessed as a class attribute to this test. It was already being implicitly tested in some of the other tests, but this feels clear and helpful. --- tests/test_base_descriptor.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/test_base_descriptor.py b/tests/test_base_descriptor.py index aeaf032b..de66f39e 100644 --- a/tests/test_base_descriptor.py +++ b/tests/test_base_descriptor.py @@ -72,5 +72,13 @@ def test_basedescriptor_fallback(): def test_basedescriptor_get(): + """Check the __get__ function works + + BaseDescriptor provides an implementation of __get__ that + returns the descriptor when accessed as a class attribute, + and calls `instance_get` when accessed as the attribute of + an instance. This test checks both those scenarios. + """ e = Example() assert e.my_property == "An example value." + assert isinstance(Example.my_property, MockProperty) From c92500f0b568e5025a1cf53c4473673412007e5c Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 30 Jul 2025 23:21:28 +0100 Subject: [PATCH 26/59] Improve test_properties.py I've added lots of docstrings and comments. I've added a few more checks to make sure the types are picked up correctly. I believe I've also fixed a race condition (#139) while I'm at it. --- tests/test_properties.py | 166 +++++++++++++++++++++++++++++++-------- 1 file changed, 132 insertions(+), 34 deletions(-) diff --git a/tests/test_properties.py b/tests/test_properties.py index c660df09..9fe11802 100644 --- a/tests/test_properties.py +++ b/tests/test_properties.py @@ -1,14 +1,17 @@ from threading import Thread +from typing import Any from pytest import raises -from pydantic import BaseModel +from pydantic import BaseModel, RootModel from fastapi.testclient import TestClient +import pytest import labthings_fastapi as lt from labthings_fastapi.exceptions import NotConnectedToServerError +from temp_client import poll_task -class TestThing(lt.Thing): +class PropertyTestThing(lt.Thing): boolprop: bool = lt.property(default=False) "A boolean property" @@ -37,31 +40,72 @@ def toggle_boolprop(self): @lt.thing_action def toggle_boolprop_from_thread(self): + """Toggle boolprop from a new threading.Thread. + + This checks we can still toggle the property from a thread + that definitely isn't a worker thread created by FastAPI. + """ t = Thread(target=self.toggle_boolprop) t.start() + # Ensure the thread has finished before the action completes: + t.join() -thing = TestThing() -server = lt.ThingServer() -server.add_thing(thing, "/thing") +@pytest.fixture +def server(): + thing = PropertyTestThing() + server = lt.ThingServer() + server.add_thing(thing, "/thing") + return server -def test_instantiation_with_type(): +def test_types_are_found(): + """Check the correct type is determined for PropertyTestThing's properties. + + Note that the special case of types that are already BaseModel subclasses + is tested in test_instantiation_with_model. """ - Check the internal model (data type) of the DataProperty descriptor is a BaseModel + T = PropertyTestThing + # BaseProperty.value_type should be the (Python) type of the property + assert T.boolprop.value_type is bool + assert T.stringprop.value_type is str + assert T.undoc.value_type is Any + assert T.floatprop.value_type is float + # BaseProperty.model should wrap that type in a RootModel + for name in ["boolprop", "stringprop", "undoc", "floatprop"]: + p = getattr(T, name) + # Check the returned model is a rootmodel + assert issubclass(p.model, RootModel) + # Check that it is wrapping the correct type + assert p.model.model_fields["root"].annotation is p.value_type + + +def test_instantiation_with_type(): + """Check the property's type is correctly wrapped in a BaseModel. To send the data over HTTP LabThings-FastAPI uses Pydantic models to describe data - types. + types. If a property is defined using simple Python types, we need to wrap them + in a `pydantic` model. The type is exposed as `.value_type` and the wrapped + model as `.model`. """ - # This will not work unless the property is assigned to a thing - class TestThing(lt.Thing): + # `prop` will not work unless the property is assigned to a thing + class Dummy(lt.Thing): prop: bool = lt.property(default=False) - assert issubclass(TestThing.prop.model, BaseModel) + @lt.property + def func_prop(self) -> bool: + return False + + assert Dummy.prop.value_type is bool + assert issubclass(Dummy.prop.model, BaseModel) + assert Dummy.func_prop.value_type is bool + assert issubclass(Dummy.func_prop.model, BaseModel) def test_instantiation_with_model() -> None: + """If a property's type is already a model, it should not be wrapped.""" + class MyModel(BaseModel): a: int = 1 b: float = 2.0 @@ -73,77 +117,131 @@ class Dummy: def func_prop(self) -> MyModel: return MyModel() - assert Dummy.prop.model is MyModel # type: ignore[attr-defined] + assert Dummy.prop.model is MyModel + assert Dummy.prop.value_type is MyModel # Dummy.prop is typed as MyModel, but it's a descriptor assert Dummy.func_prop.model is MyModel + assert Dummy.func_prop.value_type is MyModel -def test_property_get_and_set(): +def test_property_get_and_set(server): + """Use PUT and GET requests to check the property. + + PUT sets the value and GET retrieves it, so we use a PUT + to set a known value, and check it comes back when we read + it with a GET request. + """ with TestClient(server.app) as client: test_str = "A silly test string" - client.put("/thing/stringprop", json=test_str) + # Write to the property: + response = client.put("/thing/stringprop", json=test_str) + # Check for a successful response code + assert response.status_code == 201 + # Check it was written successfully after_value = client.get("/thing/stringprop") + assert after_value.status_code == 200 assert after_value.json() == test_str -def test_boolprop(): +def test_boolprop(server): + """Test that the boolean property can be read and written. + + PUT requests write to the property, and GET reads it. + """ with TestClient(server.app) as client: r = client.get("/thing/boolprop") - assert r.json() is False - client.put("/thing/boolprop", json=True) + assert r.status_code == 200 # Successful read + assert r.json() is False # Known initial value + r = client.put("/thing/boolprop", json=True) + assert r.status_code == 201 # Successful write r = client.get("/thing/boolprop") + assert r.status_code == 200 # Successful read assert r.json() is True -def test_decorator_with_no_annotation(): +def test_decorator_with_no_annotation(server): + """Test a property made with an un-annotated function.""" with TestClient(server.app) as client: r = client.get("/thing/undoc") - assert r.json() is None + assert r.status_code == 200 # Read the property OK + assert r.json() is None # The return value was None r = client.put("/thing/undoc", json="foo") - assert r.status_code != 200 + assert r.status_code == 405 # Read-only, so "method not allowed" -def test_readwrite_with_getter_and_setter(): +def test_readwrite_with_getter_and_setter(server): + """Test floatprop can be read and written with a getter/setter.""" with TestClient(server.app) as client: r = client.get("/thing/floatprop") - assert r.json() == 1.0 + assert r.status_code == 200 # Read the property OK + assert r.json() == 1.0 # Got the expected value r = client.put("/thing/floatprop", json=2.0) - assert r.status_code == 201 + assert r.status_code == 201 # Wrote to the property OK r = client.get("/thing/floatprop") - assert r.json() == 2.0 + assert r.status_code == 200 # Read the property OK + assert r.json() == 2.0 # Got the value we wrote + # We check here that writing an invalid value raises an error code: r = client.put("/thing/floatprop", json="foo") - assert r.status_code != 200 + assert r.status_code == 422 # Unprocessable entity (wrong type) + +def test_sync_action(server): + """Check that we can change a property by invoking an action. -def test_sync_action(): + This action doesn't start any extra threads. + """ with TestClient(server.app) as client: - client.put("/thing/boolprop", json=False) - r = client.get("/thing/boolprop") - assert r.json() is False + # Write to the property so it has a known value + r = client.put("/thing/boolprop", json=False) + assert r.status_code == 201 # successful write + r = client.get("/thing/boolprop") # Read it back + assert r.status_code == 200 # successful read + assert r.json() is False # the value we wrote + + # Now, we invoke the action with a POST request r = client.post("/thing/toggle_boolprop", json={}) - assert r.status_code in [200, 201] + assert r.status_code == 201 # Action started OK + # In the future, an action that completes quickly + # could return 200, which would indicate it has + # already finished. Currently, we always return + # 201 to say we started successfully - we need + # to poll the task to check it's finished. + poll_task(client, r.json()) + # Read the property after it's been toggled r = client.get("/thing/boolprop") + assert r.status_code == 200 assert r.json() is True -def test_setting_from_thread(): +def test_setting_from_thread(server): + """Repeat test_sync_action but toggle the property from a new thread. + + This checks there's nothing special about the action thread. + """ with TestClient(server.app) as client: - client.put("/thing/boolprop", json=False) + # Reset boolprop to a known state + r = client.put("/thing/boolprop", json=False) + assert r.status_code == 201 r = client.get("/thing/boolprop") + assert r.status_code == 200 assert r.json() is False r = client.post("/thing/toggle_boolprop_from_thread", json={}) - assert r.status_code in [200, 201] + assert r.status_code == 201 # Action started OK + poll_task(client, r.json()) + # Check the property changed. r = client.get("/thing/boolprop") + assert r.status_code == 200 assert r.json() is True -def test_setting_without_event_loop(): +def test_setting_without_event_loop(server): """Test that an exception is raised if updating a DataProperty without connecting the Thing to a running server with an event loop. """ # This test may need to change, if we change the intended behaviour # Currently it should never be necessary to change properties from the # main thread, so we raise an error if you try to do so + thing = PropertyTestThing() with raises(NotConnectedToServerError): thing.boolprop = False # Can't call it until the event loop's running From c6b24e72925eab419b9fcee6717f5866e1e35d48 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 30 Jul 2025 23:32:35 +0100 Subject: [PATCH 27/59] Add comments/docstrings about cacheing in get_class_attribute_docstrings --- src/labthings_fastapi/base_descriptor.py | 9 +++++++++ tests/test_base_descriptor.py | 4 ++++ 2 files changed, 13 insertions(+) diff --git a/src/labthings_fastapi/base_descriptor.py b/src/labthings_fastapi/base_descriptor.py index be558a23..9f65109b 100644 --- a/src/labthings_fastapi/base_descriptor.py +++ b/src/labthings_fastapi/base_descriptor.py @@ -295,6 +295,15 @@ class Example: just paste the example above into a Python interpreter). In that case, an empty dictionary is returned. + The same limitation means dynamically defined classes will result in + an empty dictionary. + + .. note:: + + This function uses a cache, so subsequent calls on the same class will + return a cached value. As dynamic classes are not supported, this is + not expected to be a problem. + :param cls: The class to inspect :return: A mapping of attribute names to docstrings. Note that this will be wrapped in a `types.MappingProxyType` to prevent accidental modification. diff --git a/tests/test_base_descriptor.py b/tests/test_base_descriptor.py index de66f39e..95eba9e3 100644 --- a/tests/test_base_descriptor.py +++ b/tests/test_base_descriptor.py @@ -45,6 +45,10 @@ def test_docstrings_are_cached(): """Check that the docstrings aren't being regenerated every time.""" docs1 = get_class_attribute_docstrings(Example) docs2 = get_class_attribute_docstrings(Example) + # The dictionary of attribute docstrings is cached, keyed on the + # class. The test below checks the same object is returned, not + # just one with the same values in it - this implies the cache + # is working. assert docs1 is docs2 From b3a8c6e907e4a4840cc914dcdc2ec299bf63a8a1 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 30 Jul 2025 23:36:46 +0100 Subject: [PATCH 28/59] Add a docstring for the example class. --- tests/test_base_descriptor.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/test_base_descriptor.py b/tests/test_base_descriptor.py index 95eba9e3..bf74b82d 100644 --- a/tests/test_base_descriptor.py +++ b/tests/test_base_descriptor.py @@ -16,6 +16,12 @@ def instance_get(self, _obj) -> str: # type: ignore[override] class Example: + """A class containing some attributes that may or may not have docstrings. + + We will use code in `base_descriptor` to inspect this class and test it finds + the right docstrings. + """ + my_constant: int = 10 "A number that is all mine." From b0230236c66ab48eee0f13b975ebc4e63b35571d Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 30 Jul 2025 23:45:09 +0100 Subject: [PATCH 29/59] Add docstrings to the example Thing. --- docs/source/tutorial/writing_a_thing.rst | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docs/source/tutorial/writing_a_thing.rst b/docs/source/tutorial/writing_a_thing.rst index 06d9c134..9bec6327 100644 --- a/docs/source/tutorial/writing_a_thing.rst +++ b/docs/source/tutorial/writing_a_thing.rst @@ -16,11 +16,17 @@ Our first Thing will pretend to be a light: we can set its brightness and turn i import labthings_fastapi as lt class Light(lt.Thing): + """A computer-controlled light, our first example Thing.""" + brightness: int = lt.property(default=100) + """The brightness of the light, in % of maximum.""" + is_on: bool = lt.property(default=False, readonly=true) + """Whether the light is currently on.""" @lt.action def toggle(self): + """Swap the light between on and off.""" self.is_on = not self.is_on From 70ab0ee80d4e88f51e0e5ec90c1250cfdba5c2cd Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Thu, 31 Jul 2025 09:10:35 +0100 Subject: [PATCH 30/59] Fix spelling error Co-authored-by: Beth <167304066+bprobert97@users.noreply.github.com> --- docs/source/see_also.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/source/see_also.rst b/docs/source/see_also.rst index 8fe800e3..cbb6b696 100644 --- a/docs/source/see_also.rst +++ b/docs/source/see_also.rst @@ -8,7 +8,7 @@ LabThings-FastAPI makes quite heavy use of a few key concepts from external libr Descriptors ----------- -Descriptors are a way to itercept attribute access on an object. By default, attributes of an object are just variables - so an object called ``foo`` might have an attribute called ``bar``, and you may read its value with ``foo.bar``, write its value with ``foo.bar = "baz"``, and delete the attribute with ``del foo.bar``. If ``foo`` is a descriptor, Python will call the ``__get__`` method of that descriptor when it's read and the ``__set__`` method when it's written to. You have quite probably used a descriptor already, because the built-in `~builtins.property` creates a descriptor object: that's what runs your getter method when the property is accessed. The descriptor protocol is described with plenty of examples in the `Descriptor Guide`_ in the Python documentation. +Descriptors are a way to intercept attribute access on an object. By default, attributes of an object are just variables - so an object called ``foo`` might have an attribute called ``bar``, and you may read its value with ``foo.bar``, write its value with ``foo.bar = "baz"``, and delete the attribute with ``del foo.bar``. If ``foo`` is a descriptor, Python will call the ``__get__`` method of that descriptor when it's read and the ``__set__`` method when it's written to. You have quite probably used a descriptor already, because the built-in `~builtins.property` creates a descriptor object: that's what runs your getter method when the property is accessed. The descriptor protocol is described with plenty of examples in the `Descriptor Guide`_ in the Python documentation. In LabThings-FastAPI, descriptors are used to implement :ref:`wot_actions` and :ref:`wot_properties` on `.Thing` subclasses. The intention is that these will function like standard Python methods and properties, but will also be available over HTTP, along with automatic documentation in the :ref:`wot_td` and OpenAPI documents. From 4659d592f6633fb9770496ac7d7d20f13dc4a8aa Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Thu, 31 Jul 2025 10:01:40 +0100 Subject: [PATCH 31/59] Improve tests of base_descriptor This adds checks for a couple of error conditions, and ensures all code paths are checked. It also tests that we don't end up with extra docstrings we're not expecting. --- tests/test_base_descriptor.py | 93 ++++++++++++++++++++++++++++++++++- 1 file changed, 92 insertions(+), 1 deletion(-) diff --git a/tests/test_base_descriptor.py b/tests/test_base_descriptor.py index bf74b82d..af87fa6a 100644 --- a/tests/test_base_descriptor.py +++ b/tests/test_base_descriptor.py @@ -39,12 +39,80 @@ class Example: a body giving a longer description of what's going on. """ + my_property_with_only_description = MockProperty() + """ + This is a poorly formatted docstring that does not have + a one-line title. It should result in the property name + being used as a title, and this text as description. + """ + + # This line looks like an attribute assignment with a docstring, + # but it's not - because we are not assigning to a simple name. + # This tests that such assignments won't cause errors. + my_property_with_nice_docs.attribute = "dummy value" + """A spurious docstring.""" + + # As above, this is testing that we safely ignore assignments + # that are not to simple names. The code below should not + # cause an error, but will cause a ``continue`` statement + # to skip actions, testing another code path when the + # class is analysed. + dict_attribute = {} + dict_attribute["foo"] = "bar" + """Here is a spurious docstring that should be ignored.""" + + base_descriptor = BaseDescriptor() + """This descriptor should raise NotImplementedError.""" + def test_docstrings_are_retrieved(): - """Check that the docstring can be picked up from the class definition.""" + """Check that the docstring can be picked up from the class definition. + + This test checks that: + * We get docstrings for exactly the attributes we expect. + * The one-line docstrings are picked up correctly. + * The docstring-inspection code isn't confused by spurious docstrings + next to assignments that are not to simple names. (see comments on + the class definition of `Example`). + + Detection and interpretation of multiline docstrings is tested in + `test_basedescriptor_with_good_docstring`. + """ docs = get_class_attribute_docstrings(Example) assert docs["my_constant"] == "A number that is all mine." assert docs["my_property"] == "Docs for my_property." + expected_names = [ + "my_constant", + "my_property", + "my_property_with_nice_docs", + "my_property_with_only_description", + "base_descriptor", + ] + assert set(docs.keys()) == set(expected_names) + + +def test_non_classes_raise_errors(): + """Check we validate the input object. + + If `get_class_attribute_docstrings` is called on something other than + a class, we should raise an error. + """ + + def dummy(): + pass + + with pytest.raises(TypeError): + get_class_attribute_docstrings(dummy) + + +def test_uncheckable_class(): + """Check we don't crash if we can't check a class. + + If `inspect.getsource` fails, we should return an empty dict. + """ + MyClass = type("MyClass", (), {"intattr": 10}) + doc = get_class_attribute_docstrings(MyClass) + assert doc == {} def test_docstrings_are_cached(): @@ -66,6 +134,26 @@ def test_basedescriptor_with_good_docstring(): assert prop.description.startswith("The docstring") +def test_basedescriptor_with_oneline_docstring(): + """Check we get the right documentation properties for a one-liner.""" + prop = Example.my_property + assert prop.name == "my_property" + assert prop.title == "Docs for my_property." + assert prop.description.startswith("Docs for my_property.") + + +def test_basedescriptor_with_bad_multiline_docstring(): + """Check a docstring with no title produces the expected result. + + A multiline docstring with no title (i.e. no blank second line) + should result in the whole docstring being used as the description. + """ + prop = Example.my_property_with_only_description + assert prop.name == "my_property_with_only_description" + assert prop.title == "This is a poorly formatted docstring that does not have" + assert prop.description.startswith("This is a poorly formatted") + + def test_basedescriptor_orphaned(): """Check the right error is raised if we ask for the name outside a class.""" prop = MockProperty() @@ -92,3 +180,6 @@ def test_basedescriptor_get(): e = Example() assert e.my_property == "An example value." assert isinstance(Example.my_property, MockProperty) + with pytest.raises(NotImplementedError): + # BaseDescriptor requires `instance_get` to be overridden. + e.base_descriptor From 77f154c4be6063e3ff15197f31dc9bab0bfb1f96 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Thu, 31 Jul 2025 23:07:09 +0100 Subject: [PATCH 32/59] Add a full stop Co-authored-by: Beth <167304066+bprobert97@users.noreply.github.com> --- src/labthings_fastapi/thing_property.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/labthings_fastapi/thing_property.py b/src/labthings_fastapi/thing_property.py index 2b4779bc..d04af184 100644 --- a/src/labthings_fastapi/thing_property.py +++ b/src/labthings_fastapi/thing_property.py @@ -807,7 +807,7 @@ def setting( ) -> FunctionalSetting[Value] | Value: r"""Define a Setting on a `.Thing`\ . - A setting is a property that is saved to disk + A setting is a property that is saved to disk. This function defines a setting, which is a special Property that will be saved to disk, so it persists even when the LabThings server is From c5f98d6b646879591fe53f32c06ae4d40f481e6c Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Thu, 31 Jul 2025 23:24:11 +0100 Subject: [PATCH 33/59] Apply suggestions from code review Co-authored-by: Beth <167304066+bprobert97@users.noreply.github.com> --- src/labthings_fastapi/base_descriptor.py | 4 ++-- src/labthings_fastapi/thing_property.py | 5 +++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/labthings_fastapi/base_descriptor.py b/src/labthings_fastapi/base_descriptor.py index 9f65109b..43b7cd26 100644 --- a/src/labthings_fastapi/base_descriptor.py +++ b/src/labthings_fastapi/base_descriptor.py @@ -28,7 +28,7 @@ class DescriptorNotAddedToClassError(RuntimeError): """Descriptor has not yet been added to a class. This error is raised if certain properties of descriptors are accessed - before ``__set_name__`` has been called on the descriptor. ``__set_name`` + before ``__set_name__`` has been called on the descriptor. ``__set_name__`` is part of the descriptor protocol, and is called when a class is defined to notify the descriptor of its name and owning class. @@ -122,7 +122,7 @@ def __set_name__(self, owner: type[Thing], name: str) -> None: if name in attr_docs: self.__doc__ = attr_docs[name] - def assert_set_name_called(self): + def assert_set_name_called(self) -> None: """Raise an exception if ``__set_name__`` has not yet been called. :raises DescriptorNotAddedToClassError: if ``__set_name__`` has not yet diff --git a/src/labthings_fastapi/thing_property.py b/src/labthings_fastapi/thing_property.py index d04af184..572f131d 100644 --- a/src/labthings_fastapi/thing_property.py +++ b/src/labthings_fastapi/thing_property.py @@ -15,7 +15,7 @@ class Counter(lt.Thing): "A counter that knows what's remaining." count: int = lt.property(default=0, readonly=True) - "The number of times we've increnented the counter." + "The number of times we've incremented the counter." target: int = lt.property(default=10) "The number of times to increment before we stop." @@ -184,7 +184,8 @@ def default_factory_from_arguments( """ if default is ... and default_factory is None: # If the default is an ellipsis, we have no default value. - # This is not allowed for DataProperty, so we raise an error. + # Not having a default_factory alongside this + # is not allowed for DataProperty, so we raise an error. raise MissingDefaultError() if default is not ... and default_factory is not None: # If both default and default_factory are set, we raise an error. From 9c1de799d8122c7bcccbdfcbf68fad7f50dbdebd Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Thu, 31 Jul 2025 23:38:56 +0100 Subject: [PATCH 34/59] Apply suggestions from code review Co-authored-by: Beth <167304066+bprobert97@users.noreply.github.com> --- typing_tests/thing_definitions.py | 1 + 1 file changed, 1 insertion(+) diff --git a/typing_tests/thing_definitions.py b/typing_tests/thing_definitions.py index 3c54f476..516cfbeb 100644 --- a/typing_tests/thing_definitions.py +++ b/typing_tests/thing_definitions.py @@ -59,6 +59,7 @@ class TestPropertyDefaultsMatch(lt.Thing): optionalintprop: int | None = lt.property(default=None) optionalintprop2: int | None = lt.property(default=0) optionalintprop3: int | None = lt.property(default_factory=optional_int_factory) + optionalintprop4: int | None = lt.property(default_factory=int_factory) # This property should cause mypy to throw an error, as the default is a string. # The type hint is an int, so this should cause a mypy error. From 8b18d754bc3e5162d6849e0e7894cacb85d9f370 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Fri, 1 Aug 2025 01:14:17 +0100 Subject: [PATCH 35/59] Better error if default_factory is not callable. I've also added whitespace between overloads, fixed a docstring, and added a test for overspecified arguments. --- src/labthings_fastapi/thing_property.py | 49 +++++++++++++++++++++---- 1 file changed, 42 insertions(+), 7 deletions(-) diff --git a/src/labthings_fastapi/thing_property.py b/src/labthings_fastapi/thing_property.py index 572f131d..243b5e34 100644 --- a/src/labthings_fastapi/thing_property.py +++ b/src/labthings_fastapi/thing_property.py @@ -160,6 +160,10 @@ def default_factory_from_arguments( and will either return the ``default_factory`` if it is provided, or will wrap the default value provided in a factory function. + Note that this wrapping does not copy the default value each time it is + called, so mutable default values are **only** safe if supplied as a + factory function. + This is used to avoid repeating the logic of checking whether a default value or a factory function has been provided, and it returns a factory rather than a default value so that it may be called multiple times to @@ -195,7 +199,8 @@ def default_factory_from_arguments( def default_factory() -> Value: return default - assert callable(default_factory), "The default_factory must be callable." + if not callable(default_factory): + raise MissingDefaultError("The default_factory must be callable.") return default_factory @@ -204,12 +209,20 @@ def default_factory() -> Value: def property( # noqa: D103 getter: Callable[[Any], Value], ) -> FunctionalProperty[Value]: ... -@overload # use as `field: int = property(0)`` -def property(*, default: Value, readonly: bool = False) -> Value: ... # noqa: D103 -@overload # use as `field: int = property(default_factory=lambda: 0)` # noqa: E302 + + +@overload # use as `field: int = property(default=0)` +def property( # noqa: D103 + *, default: Value, readonly: bool = False +) -> Value: ... + + +@overload # use as `field: int = property(default_factory=lambda: 0)` def property( # noqa: D103 *, default_factory: Callable[[], Value], readonly: bool = False ) -> Value: ... + + def property( getter: ValueGetter | EllipsisType = ..., *, @@ -253,6 +266,8 @@ def property( :raises MissingDefaultError: if no valid default value is supplied, and a getter is not in use. + :raises OverspecifiedDefaultError: if the default is specified more + than once (e.g. ``default``, ``default_factory``, or ``getter``). **Typing Notes** @@ -288,6 +303,11 @@ def property( "this means the default value was not passed as a keyword " "argument, which is required." ) + if default_factory or default is not ...: + raise OverspecifiedDefaultError( + "A getter was specified at the same time as a default. Only " + "one of a getter, default, and default_factory may be used." + ) return FunctionalProperty( fget=getter, ) @@ -434,10 +454,12 @@ class DataProperty(BaseProperty[Value], Generic[Value]): def __init__( # noqa: D105,D107,DOC101,DOC103 self, default: Value, *, readonly: bool = False ) -> None: ... + @overload def __init__( # noqa: D105,D107,DOC101,DOC103 self, *, default_factory: ValueFactory, readonly: bool = False ) -> None: ... + def __init__( self, default: Value | EllipsisType = ..., @@ -793,12 +815,20 @@ def __set__(self, obj: Thing, value: Value): def setting( # noqa: D103 getter: Callable[[Any], Value], ) -> FunctionalSetting[Value]: ... -@overload # use as `field: int = setting(0)`` -def setting(*, default: Value, readonly: bool = False) -> Value: ... # noqa: D103 + + +@overload # use as `field: int = setting(default=0)`` +def setting( # noqa: D103 + *, default: Value, readonly: bool = False +) -> Value: ... + + @overload # use as `field: int = setting(default_factory=lambda: 0)` def setting( # noqa: D103 *, default_factory: Callable[[], Value], readonly: bool = False ) -> Value: ... + + def setting( getter: ValueGetter | EllipsisType = ..., *, @@ -865,10 +895,15 @@ def setting( # without arguments. if not callable(getter): raise MissingDefaultError( - "A non-callable getter was passed to `property`. Usually," + "A non-callable getter was passed to `setting`. Usually," "this means the default value was not passed as a keyword " "argument, which is required." ) + if default_factory or default is not ...: + raise OverspecifiedDefaultError( + "A getter was specified at the same time as a default. Only " + "one of a getter, default, and default_factory may be used." + ) return FunctionalSetting( fget=getter, ) From 43e436f920a8e08cb4f6bde5252fccf603a1d447 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Fri, 1 Aug 2025 01:17:24 +0100 Subject: [PATCH 36/59] Remove deleter This was added when I was subclassing `property` but is no longer needed. --- src/labthings_fastapi/thing_property.py | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/src/labthings_fastapi/thing_property.py b/src/labthings_fastapi/thing_property.py index 243b5e34..29e76b5e 100644 --- a/src/labthings_fastapi/thing_property.py +++ b/src/labthings_fastapi/thing_property.py @@ -697,14 +697,6 @@ def fset(self) -> ValueSetter | None: # noqa: DOC201 """The setter function.""" # noqa: D401 return self._fset - @builtins.property - def fdel(self) -> None: # noqa: DOC201 - """The deleter function. - - This function always returns ``None`` as deleters are not yet supported. - """ # noqa: D401 - return None - def getter(self, fget: ValueGetter) -> Self: """Set the getter function of the property. @@ -778,18 +770,6 @@ def set_myprop(self, val: int) -> None: self.readonly = False return self - def deleter(self, fdel: Callable) -> Self: - """Set a deleter function. Currently unsupported. - - :param fdel: The function called when the attribute is deleted. - :return: The descriptor (i.e. ``self``). - - :raises NotImplementedError: every time, because it is not supported. - """ - raise NotImplementedError( - "Deleter functions are not supported for FunctionalProperty." - ) - def instance_get(self, obj: Thing) -> Value: """Get the value of the property. From f757a9554ee4fd1782de4d46be6bf0db738abcf7 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Fri, 1 Aug 2025 01:22:18 +0100 Subject: [PATCH 37/59] Apply suggestions from code review Co-authored-by: Beth <167304066+bprobert97@users.noreply.github.com> --- src/labthings_fastapi/thing_property.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/labthings_fastapi/thing_property.py b/src/labthings_fastapi/thing_property.py index 29e76b5e..2f89b801 100644 --- a/src/labthings_fastapi/thing_property.py +++ b/src/labthings_fastapi/thing_property.py @@ -921,7 +921,7 @@ class DataSetting(DataProperty[Value], BaseSetting[Value], Generic[Value]): A setting can be accessed via the HTTP API and is persistent between sessions. - A `.ThingSetting` is a `.DataProperty` with extra functionality for triggering + A `.DataSetting` is a `.DataProperty` with extra functionality for triggering a `.Thing` to save its settings. Note: If a setting is mutated rather than assigned to, this will not trigger saving. @@ -965,7 +965,7 @@ class FunctionalSetting(FunctionalProperty[Value], BaseSetting[Value], Generic[V A setting can be accessed via the HTTP API and is persistent between sessions. - A `.ThingSetting` is a `.FunctionalProperty` with extra functionality for + A `.FunctionalSetting` is a `.FunctionalProperty` with extra functionality for triggering a `.Thing` to save its settings. Note: If a setting is mutated rather than assigned to, this will not trigger From ab7a63139ea9ef3bd5efea903a65d3cc880b0db7 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Fri, 1 Aug 2025 01:35:16 +0100 Subject: [PATCH 38/59] Improve docstrings I've mentioned why the default for the getter is ... and mentioned all exceptions raised. --- src/labthings_fastapi/thing_property.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/labthings_fastapi/thing_property.py b/src/labthings_fastapi/thing_property.py index 2f89b801..88239c3a 100644 --- a/src/labthings_fastapi/thing_property.py +++ b/src/labthings_fastapi/thing_property.py @@ -292,7 +292,9 @@ def property( Finally, the type of the ``default`` argument includes `.EllipsisType` so that we can use ``...`` as its default value. This allows us to distinguish between ``default`` not being set (``...``) and a desired - default value of ``None``. + default value of ``None``. Similarly, ``...`` is the default value for + ``getter`` so we can raise a more helpful error if a non-callable + value is passed as the first argument. """ if getter is not ...: # If the default is callable, we're being used as a decorator @@ -863,12 +865,14 @@ def setting( :return: a setting descriptor. - :raises MissingDefaultError: if no valid default is supplied. + :raises MissingDefaultError: if no valid default or getter is supplied. + :raises OverspecifiedDefaultError: if the default is specified more + than once (e.g. ``default``, ``default_factory``, or ``getter``). **Typing Notes** - The return type of this function is a "white lie" in order to allow - dataclass-style type annotations + See the typing notes on `.property` as they all apply to `.setting` as + well. """ if getter is not ...: # If the default is callable, we're being used as a decorator From 089b98e7bdf266c5ac52e981d9ea027226b5f582 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Sat, 2 Aug 2025 21:52:32 +0100 Subject: [PATCH 39/59] Add tests of `property` and `setting` This checks the two top-level functions have the same interface. --- tests/test_property.py | 218 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 218 insertions(+) create mode 100644 tests/test_property.py diff --git a/tests/test_property.py b/tests/test_property.py new file mode 100644 index 00000000..61cad896 --- /dev/null +++ b/tests/test_property.py @@ -0,0 +1,218 @@ +"""Test `lt.property` and its associated classes. + +This is a new test module, intended to test individual bits of code, +rather than check the whole property mechanism at once. This should +mean this module is more bottom-up than the old +`test_properties.py`. Currently, both are part of the test suite, +as it's helpful to take both approaches. + +This module currently focuses on checking the top level functions, +in particular checking `lt.property` and `lt.setting` work in the +same way. +""" + +import fastapi +from fastapi.testclient import TestClient +import pydantic +import pytest +from labthings_fastapi import thing_property as tp + + +def test_default_factory_from_arguments(): + """Check the function that implements default/default_factory + + behaves correctly. It should always return a function that + returns a default value, and should error if both arguments + are provided, or if none are provided. + """ + # Check for an error with no arguments + with pytest.raises(tp.MissingDefaultError): + tp.default_factory_from_arguments() + + # Check for an error with both arguments + with pytest.raises(tp.OverspecifiedDefaultError): + tp.default_factory_from_arguments([], list) + + # Check a factory is passed unchanged + assert tp.default_factory_from_arguments(..., list) is list + + # Check a value is wrapped in a factory + factory = tp.default_factory_from_arguments(True, None) + assert factory() is True + + # Check there's an error if our default factory isn't callable + with pytest.raises(tp.MissingDefaultError): + tp.default_factory_from_arguments(default_factory=False) + + # Check None works as a default value + factory = tp.default_factory_from_arguments(default=None) + assert factory() is None + + +class ArgCapturer: + """A class that remembers its init arguments.""" + + def __init__(self, *args, **kwargs): + """Store arguments for later inspection.""" + self.args = args + self.kwargs = kwargs + + +def mock_and_capture_args(monkeypatch, target, name): + """Replace a class with an ArgCapturer + + A dynamically created subclass will be swapped in for the + specified class, allowing its arguments to be checked. + + :param monkeypatch: the pytest fixture. + :param target: the module where the class is defined. + :param name: the class name. + """ + MockClass = type( + name, + (ArgCapturer,), + {}, + ) + monkeypatch.setattr(target, name, MockClass) + + +@pytest.mark.parametrize("func", [tp.property, tp.setting]) +def test_toplevel_function(monkeypatch, func): + """Test the various ways in which `lt.property` or `lt.setting` may be invoked. + + This test is parametrized, so `func` will be either `tp.property` or `tp.setting`. + We then look up the corresponding descriptor classes. + + It's unfortunate that the body of this test is a bit generic, but as both + functions should work identically, it's worth ensuring they are tested the same. + + This is intended only to test that `func` invokes the classes correctly, + so they are mocked. + """ + # Mock DataProperty,FunctionalProperty or the equivalent for settings + # suffix will be "Property" or "Setting" + suffix = func.__name__.capitalize() + mock_and_capture_args(monkeypatch, tp, f"Data{suffix}") + mock_and_capture_args(monkeypatch, tp, f"Functional{suffix}") + DataClass = getattr(tp, f"Data{suffix}") + FunctionalClass = getattr(tp, f"Functional{suffix}") + + def getter(self) -> str: + return "test" + + # This is equivalent to use as a decorator + prop = func(getter) + # The decorator should instantiate a FunctionalProperty/FunctionalSetting + assert isinstance(prop, FunctionalClass) + assert prop.args == () + assert prop.kwargs == {"fget": getter} + + # When instantiated with a default, we make a + # DataProperty/DataSetting. Note that we convert the default + # to a datafactory using `default_factory_from_arguments` + # so the class gets a default factory.` + prop = func(default=0) + assert isinstance(prop, DataClass) + assert prop.args == () + assert prop.kwargs["default_factory"]() == 0 + assert prop.kwargs["readonly"] is False + assert len(prop.kwargs) == 2 + + # The same thing should happen when we use a factory, + # except it should pass through the factory function unchanged. + prop = func(default_factory=list) + assert isinstance(prop, DataClass) + assert prop.args == () + assert prop.kwargs["default_factory"] is list + assert prop.kwargs["readonly"] is False + assert len(prop.kwargs) == 2 + + # The positional argument is the setter, so `None` is not valid + # and probably means someone forgot to add `default=`. + with pytest.raises(tp.MissingDefaultError): + func(None) + + # Calling with no arguments is also not valid and raises an error + with pytest.raises(tp.MissingDefaultError): + func() + + # If more than one default is specified, we should raise an error. + with pytest.raises(tp.OverspecifiedDefaultError): + func(default=[], default_factory=list) + with pytest.raises(tp.OverspecifiedDefaultError): + func(getter, default=[]) + with pytest.raises(tp.OverspecifiedDefaultError): + func(getter, default_factory=list) + + +def test_baseproperty_type_and_model(): + """Test type functionality in BaseProperty + + This checks baseproperty correctly wraps plain types in a + `pydantic.RootModel`. + """ + prop = tp.BaseProperty() + + # By default, we have no type so `.type` errors. + with pytest.raises(tp.MissingTypeError): + prop.value_type + with pytest.raises(tp.MissingTypeError): + prop.model + + # Once _type is set, these should both work. + prop._type = str | None + assert str(prop.value_type) == "str | None" + assert issubclass(prop.model, pydantic.RootModel) + assert str(prop.model.model_fields["root"].annotation) == "str | None" + + +def test_baseproperty_type_and_model_pydantic(): + """Test type functionality in BaseProperty + + This checks baseproperty behaves correctly when its + type is a BaseModel instance. + """ + prop = tp.BaseProperty() + + class MyModel(pydantic.BaseModel): + foo: str + bar: int + + # Once _type is set, these should both work. + prop._type = MyModel + assert prop.value_type is MyModel + assert prop.model is MyModel + + +def test_baseproperty_add_to_fastapi(): + """Check the method that adds the property to the HTTP API.""" + # Subclass to add __set__ (which is missing on BaseProperty as it's + # implemented by subclasses). + + class MyProperty(tp.BaseProperty): + def __set__(self, obj, val): + pass + + class Example: + prop = MyProperty() + """A docstring with a title. + + A docstring body. + """ + prop._type = str | None + + # Add a path attribute, so we can use Example as a mock Thing. + path = "/example/" + + # Make a FastAPI app and retrieve the OpenAPI document + app = fastapi.FastAPI() + Example.prop.add_to_fastapi(app, Example()) + with TestClient(app) as tc: + r = tc.get("/openapi.json") + assert r.status_code == 200 + openapi = r.json() + + # Check the property appears at the expected place + entry = openapi["paths"]["/example/prop"] + # Check it declares the right methods + assert set(entry.keys()) == set(["get", "put"]) From 416e9b0163e8d228734b755d87974860fd78a4f9 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Sat, 2 Aug 2025 21:52:50 +0100 Subject: [PATCH 40/59] Test what happens if getter and setter have different names. --- tests/test_base_descriptor.py | 70 ++++++++++++++++++++++++++++++++++- 1 file changed, 69 insertions(+), 1 deletion(-) diff --git a/tests/test_base_descriptor.py b/tests/test_base_descriptor.py index af87fa6a..83a31f91 100644 --- a/tests/test_base_descriptor.py +++ b/tests/test_base_descriptor.py @@ -11,7 +11,8 @@ class MockProperty(BaseDescriptor[str]): # The line below isn't defined on a `Thing`, so mypy # errors - but we ignore this for testing. - def instance_get(self, _obj) -> str: # type: ignore[override] + def instance_get(self, _obj) -> str: + """This is called by BaseProperty.__get__.""" return "An example value." @@ -183,3 +184,70 @@ def test_basedescriptor_get(): with pytest.raises(NotImplementedError): # BaseDescriptor requires `instance_get` to be overridden. e.base_descriptor + + +class MockFunctionalProperty(MockProperty): + """A mock property class with a setter decorator. + + This class is used by test_decorator_different_names. + """ + + def __init__(self, fget): + """Add a mock getter and initialise variables.""" + super().__init__() + self._getter = fget + self._setter = None + self._names = [] + + def setter(self, fset): + """Can be used as a decorator to add a setter.""" + self._setter = fset + return self + + def __set_name__(self, owner, name): + """Check how many times __set_name__ is called.""" + self._names.append(name) + super().__set_name__(owner, name) + + +class DecoratorExample: + """A class that has decorators defined on it. + + This class is used by test_decorator_different_names. + """ + + @MockFunctionalProperty + def prop1(self): + """A getter function that returns a string.""" + return "prop1 value" + + @prop1.setter + def prop1(self, _val): + """A getter for prop1, with the same name""" + pass + + @MockFunctionalProperty + def prop2(self): + """A getter function that returns a string.""" + return "prop2 value" + + @prop2.setter + def set_prop2(self, val): + """A setter that is not called prop2""" + pass + + +def test_decorator_different_names(): + """Test what happens when `lt.property` is used as a decorator. + + `mypy` gets confused by the property-style syntax of decorating a + getter and setter with the same name. This test checks what happens + when a getter and setter with different names are used. + + There is potential for confusion here in `__set_name__`. + """ + E = DecoratorExample + assert E.prop1.name == "prop1" + assert E.prop1._names == ["prop1"] + assert E.prop2.name == "prop2" + assert E.prop2._names == ["prop2", "set_prop2"] From 354cdfb00d923b30d7bb215d82854b03f38e7d4a Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 4 Aug 2025 09:44:41 +0100 Subject: [PATCH 41/59] Guard against BaseDescriptor being used twice. This adds, and tests, code to raise errors if descriptor instances are shared by multiple attributes. A special case will be needed in `FunctionalProperty.__set_name__`. --- src/labthings_fastapi/base_descriptor.py | 93 ++++++++++++++++++++++- tests/test_base_descriptor.py | 94 ++++++++++++++++-------- 2 files changed, 155 insertions(+), 32 deletions(-) diff --git a/src/labthings_fastapi/base_descriptor.py b/src/labthings_fastapi/base_descriptor.py index 43b7cd26..e8dcb81c 100644 --- a/src/labthings_fastapi/base_descriptor.py +++ b/src/labthings_fastapi/base_descriptor.py @@ -52,6 +52,89 @@ class Test(lt.Thing): """ +class DescriptorAddedToClassTwiceError(RuntimeError): + """A Descriptor has been added to a class more than once. + + This error is raised if ``__set_name__`` is called more than once on a + descriptor. This happens when either the same descriptor instance is + used twice in one class definition, or if a descriptor instance is used + on more than one class. + + .. note:: + + `.FunctionalProperty` includes a special case that will ignore the + ``__set_name__`` call corresponding to the setter. This allows the + property to be defined like ``prop4`` below, even though it does + assign the descriptor to two names. That behaviour is specific to + `.FunctionalProperty` and `.FunctionalSetting` and is not part of + `.BaseDescriptor` because `.BaseDescriptor` has no setter. + + ``mypy`` does not allow custom property-like descriptors to follow the + syntax used by the built-in ``property`` of giving both the getter and + setter functions the same name: this causes an error because it is + a redefinition. We suggest using a different name for the setter to + work around this, hence the need for an exception. + + .. code-block:: python + + class MyDescriptor(BaseDescriptor): + "An example descriptor that inherits from BaseDescriptor." + + def __init__(getter=None): + "Initialise the descriptor, allowing use as a decorator." + self._getter = getter + + def setter(self, setter): + "Add a setter to the descriptor." + self._setter = setter + return self + + + class Example: + "An example class with descriptors." + + # prop1 is fine - only used once. + prop1 = MyDescriptor() + + # prop2 reuses the name ``prop2`` which may confuse ``mypy`` but + # will only call ``__set_name__`` once. + @MyDescriptor + def prop2(self): + "A dummy property" + return False + + @prop2.setter + def prop2(self, val): + "Set the dummy property" + pass + + # prop3a and prop3b will cause this error + prop3a = MyDescriptor() + prop3b = MyDescriptor() + + # prop4 and set_prop4 will cause this error on BaseDescriptor + # but there is a specific exception in FunctionalProperty + # to allow this form. + @MyDescriptor + def prop4(self): + "An example property with two names" + return True + + @prop4.setter + def set_prop4(self, val): + "A setter for prop4 that is not named prop4." + pass + + .. note:: + + Because this exception is raised in ``__set_name__`` it will not + appear to come from the descriptor assignment, but instead it will + be raised at the end of the class definition. The descriptor name(s) + should be in the error message. + + """ + + class BaseDescriptor(Generic[Value]): r"""A base class for descriptors in LabThings-FastAPI. @@ -95,6 +178,7 @@ def __init__(self) -> None: # We explicitly check when __set_name__ is called, so we can raise helpful # errors self._set_name_called: bool = False + self._owner_name: str = "" def __set_name__(self, owner: type[Thing], name: str) -> None: r"""Take note of the name to which the descriptor is assigned. @@ -111,10 +195,17 @@ def __set_name__(self, owner: type[Thing], name: str) -> None: :param owner: the `.Thing` subclass to which we are being attached. :param name: the name to which we have been assigned. """ + if self._set_name_called: + raise DescriptorAddedToClassTwiceError( + f"The descriptor {self._name} on {self._owner_name} has been " + f"added to a class a second time ({owner.__qualname__}.{name}). " + "This descriptor may only be added to a class once." + ) # Remember the name to which we're assigned. Accessed by the read only # property ``name``. - self._name = name self._set_name_called = True + self._name = name + self._owner_name = owner.__qualname__ # Check for docstrings on the owning class, and retrieve the one for # this attribute (identified by `name`). diff --git a/tests/test_base_descriptor.py b/tests/test_base_descriptor.py index 83a31f91..242a8f4a 100644 --- a/tests/test_base_descriptor.py +++ b/tests/test_base_descriptor.py @@ -2,6 +2,7 @@ from labthings_fastapi.base_descriptor import ( BaseDescriptor, DescriptorNotAddedToClassError, + DescriptorAddedToClassTwiceError, get_class_attribute_docstrings, ) @@ -210,44 +211,75 @@ def __set_name__(self, owner, name): super().__set_name__(owner, name) -class DecoratorExample: - """A class that has decorators defined on it. +def test_decorator_different_names(): + """Check that adding a descriptor to a class twice raises the right error. - This class is used by test_decorator_different_names. + Much confusion will result if a ``BaseDescriptor`` is added to a class twice + or added to two different classes. This test checks an error is raised when + that happens. + + Note that there is an exception to this in `.FunctionalProperty` and that + exception is tested in ``test_property.py`` in this folder. """ + # First, very obviously double-assign a BaseDescriptor + with pytest.raises(DescriptorAddedToClassTwiceError) as excinfo: - @MockFunctionalProperty - def prop1(self): - """A getter function that returns a string.""" - return "prop1 value" + class ExplicitExample: + """An example class.""" - @prop1.setter - def prop1(self, _val): - """A getter for prop1, with the same name""" - pass + prop1 = BaseDescriptor() + prop2 = prop1 - @MockFunctionalProperty - def prop2(self): - """A getter function that returns a string.""" - return "prop2 value" + # The exception occurs at the end of the class definition, so check we include + # the property names. + assert "prop1" in str(excinfo.value) + assert "prop2" in str(excinfo.value) - @prop2.setter - def set_prop2(self, val): - """A setter that is not called prop2""" - pass + # The next form of properties works and doesn't trigger the error, but is + # flagged (arguably spuriously) as an error by mypy. + class ValidExceptInMyPy: + """An example class that fails type checking but is valid Python.""" + @MockFunctionalProperty + def prop1(self): + return False -def test_decorator_different_names(): - """Test what happens when `lt.property` is used as a decorator. + @prop1.setter + def prop1(self, val): + pass - `mypy` gets confused by the property-style syntax of decorating a - getter and setter with the same name. This test checks what happens - when a getter and setter with different names are used. + # This workaround satisfies MyPy but double-assigns the descriptor. + # It should raise an error here, but is a special case in + # `.FunctionalProperty.__set_name__` so will be OK for `.FunctionalProperty` + # and `.FunctionalSetting` as a result. + with pytest.raises(DescriptorAddedToClassTwiceError) as excinfo: - There is potential for confusion here in `__set_name__`. - """ - E = DecoratorExample - assert E.prop1.name == "prop1" - assert E.prop1._names == ["prop1"] - assert E.prop2.name == "prop2" - assert E.prop2._names == ["prop2", "set_prop2"] + class DecoratorExample: + """Another example class.""" + + @MockFunctionalProperty + def prop1(self): + return False + + @prop1.setter + def set_prop1(self, val): + pass + + # The exception occurs at the end of the class definition, so check we include + # the property names. + assert "prop1" in str(excinfo.value) + assert "set_prop1" in str(excinfo.value) + + # For good measure, check re-use across classes is also prevented. + class FirstExampleClass: + prop = BaseDescriptor() + + with pytest.raises(DescriptorAddedToClassTwiceError) as excinfo: + + class SecondExampleClass: + prop = FirstExampleClass.prop + + # The message should mention names and classes + assert "prop" in str(excinfo.value) + assert "FirstExampleClass" in str(excinfo.value) + assert "SecondExampleClass" in str(excinfo.value) From 04739527a8441c48efa58fd0ed9e4c365d818a7d Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 4 Aug 2025 10:19:10 +0100 Subject: [PATCH 42/59] WIP: Add an exemption to allow properties with differently named setters This may yet have confusing side effects. --- src/labthings_fastapi/thing_property.py | 51 +++++++++++++++++++++++++ tests/test_property.py | 33 ++++++++++++++++ 2 files changed, 84 insertions(+) diff --git a/src/labthings_fastapi/thing_property.py b/src/labthings_fastapi/thing_property.py index 88239c3a..00f73a62 100644 --- a/src/labthings_fastapi/thing_property.py +++ b/src/labthings_fastapi/thing_property.py @@ -689,6 +689,57 @@ def __init__( self._fset: ValueSetter | None = None self.readonly: bool = True + def __set_name__(self, owner: type[Thing], name: str) -> None: + r"""Check for double-assignment and avoid confusing behaviour. + + This function enables code that looks like the following: + + .. code-block:: python + + import labthings_fastapi as lt + + + class Example(lt.Thing): + "An example class with a property" + + @lt.property + def prop(self) -> bool: + "An example property." + return True + + @prop.setter + def set_prop(self, val: bool) -> None: + "A setter that is not named `prop` to avoid mypy errors" + pass + + ``mypy`` does not allow custom property-like descriptors to follow the + syntax used by the built-in ``property`` of giving both the getter and + setter functions the same name: this causes an error because it is + a redefinition. We suggest using a different name for the setter to + work around this. However, that then means the same descriptor appears + as two different attributes, e.g. ``prop1`` and ``set_prop1``\ . + + This function tests for that special case, and uses the first + (i.e. getter's) name for :ref:`gen_docs` and does not raise an error. + + See `.DescriptorAddedToClassTwiceError` for more details. + + :param owner: the `.Thing` on which we are defined. + :param name: the name of the attribute to which we are assigned. + """ + if self._set_name_called: + if ( + owner.__qualname__ == self._owner_name + and self.fset + and name == self.fset.__name__ + and self.name == self.fget.__name__ + ): + # `__set_name__` is being called because of a differently-named + # setter. This should be allowed. We won't call super, as it has + # already been called for the getter + return + return super().__set_name__(owner, name) + @builtins.property def fget(self) -> ValueGetter: # noqa: DOC201 """The getter function.""" # noqa: D401 diff --git a/tests/test_property.py b/tests/test_property.py index 61cad896..09b8ebab 100644 --- a/tests/test_property.py +++ b/tests/test_property.py @@ -16,6 +16,7 @@ import pydantic import pytest from labthings_fastapi import thing_property as tp +from labthings_fastapi.base_descriptor import DescriptorAddedToClassTwiceError def test_default_factory_from_arguments(): @@ -216,3 +217,35 @@ class Example: entry = openapi["paths"]["/example/prop"] # Check it declares the right methods assert set(entry.keys()) == set(["get", "put"]) + + +def test_decorator_exception(): + r"""Check decorators work as expected when the setter has a different name. + + This is done to satisfy ``mypy`` and more information is in the + documentation for `.property`\ , `.DescriptorAddedToClassTwiceError` + and `.FunctionalProperty.__set_name__`\ . + """ + # The exception should be specific - a simple double assignment is + # still an error + with pytest.raises(DescriptorAddedToClassTwiceError): + + class BadExample: + """A class with a wrongly reused descriptor.""" + + prop1: int = tp.property(default=0) + prop2: int = prop1 + + # The example below should be exempted from the rule, i.e. no error + class Example: + @tp.property + def prop(self) -> bool: + """An example getter.""" + + @prop.setter + def set_prop(self, val: bool) -> None: + """A setter named differently.""" + pass + + assert Example.prop.name == "prop" + assert Example.prop is Example.set_prop From 25a42e576a3048cbb5b03c45905fb36ebc6c22da Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 4 Aug 2025 10:59:30 +0100 Subject: [PATCH 43/59] Better handling of differently named setters. After writing #164 I realised it was much cleaner to simply avoid returning a descriptor from `FunctionalProperty.setter` if the names didn't match. This means there will only ever be one descriptor object so we don't confuse code that looks for descriptors. --- src/labthings_fastapi/base_descriptor.py | 3 ++ src/labthings_fastapi/thing_property.py | 65 +++++------------------- tests/test_property.py | 4 +- 3 files changed, 19 insertions(+), 53 deletions(-) diff --git a/src/labthings_fastapi/base_descriptor.py b/src/labthings_fastapi/base_descriptor.py index e8dcb81c..374477cc 100644 --- a/src/labthings_fastapi/base_descriptor.py +++ b/src/labthings_fastapi/base_descriptor.py @@ -194,6 +194,9 @@ def __set_name__(self, owner: type[Thing], name: str) -> None: :param owner: the `.Thing` subclass to which we are being attached. :param name: the name to which we have been assigned. + + :raises DescriptorAddedToClassTwiceError: if the descriptor has been + assigned to two class attributes. """ if self._set_name_called: raise DescriptorAddedToClassTwiceError( diff --git a/src/labthings_fastapi/thing_property.py b/src/labthings_fastapi/thing_property.py index 00f73a62..a16ddd3b 100644 --- a/src/labthings_fastapi/thing_property.py +++ b/src/labthings_fastapi/thing_property.py @@ -689,57 +689,6 @@ def __init__( self._fset: ValueSetter | None = None self.readonly: bool = True - def __set_name__(self, owner: type[Thing], name: str) -> None: - r"""Check for double-assignment and avoid confusing behaviour. - - This function enables code that looks like the following: - - .. code-block:: python - - import labthings_fastapi as lt - - - class Example(lt.Thing): - "An example class with a property" - - @lt.property - def prop(self) -> bool: - "An example property." - return True - - @prop.setter - def set_prop(self, val: bool) -> None: - "A setter that is not named `prop` to avoid mypy errors" - pass - - ``mypy`` does not allow custom property-like descriptors to follow the - syntax used by the built-in ``property`` of giving both the getter and - setter functions the same name: this causes an error because it is - a redefinition. We suggest using a different name for the setter to - work around this. However, that then means the same descriptor appears - as two different attributes, e.g. ``prop1`` and ``set_prop1``\ . - - This function tests for that special case, and uses the first - (i.e. getter's) name for :ref:`gen_docs` and does not raise an error. - - See `.DescriptorAddedToClassTwiceError` for more details. - - :param owner: the `.Thing` on which we are defined. - :param name: the name of the attribute to which we are assigned. - """ - if self._set_name_called: - if ( - owner.__qualname__ == self._owner_name - and self.fset - and name == self.fset.__name__ - and self.name == self.fget.__name__ - ): - # `__set_name__` is being called because of a differently-named - # setter. This should be allowed. We won't call super, as it has - # already been called for the getter - return - return super().__set_name__(owner, name) - @builtins.property def fget(self) -> ValueGetter: # noqa: DOC201 """The getter function.""" # noqa: D401 @@ -765,7 +714,7 @@ def getter(self, fget: ValueGetter) -> Self: return self def setter(self, fset: ValueSetter) -> Self: - """Set the setter function of the property. + r"""Set the setter function of the property. This function returns the descriptor, so it may be used as a decorator. @@ -818,9 +767,21 @@ def set_myprop(self, val: int) -> None: must be silenced manually. We suggest using a different name for the setter as an alternative to adding ``# type: ignore[no-redef]`` to the setter function. + + It will cause problems elsewhere in the code if descriptors are assigned + to more than one attrubute, and this is checked in + `.BaseDescriptor.__set_name__`\ . We therefore return the setter rather + than the descriptor if the names don't match. The type hint does not + reflect this, as it would cause problems when the names do match (the + descriptor would become a ``FunctionalProperty | Callable`` and thus + typing errors would happen whenever it's accessed). """ self._fset = fset self.readonly = False + if fset.__name__ != self.fget.__name__: + # Don't return the descriptor if it's named differently. + # see typing notes in docstring. + return fset # type: ignore[return-value] return self def instance_get(self, obj: Thing) -> Value: diff --git a/tests/test_property.py b/tests/test_property.py index 09b8ebab..28538d78 100644 --- a/tests/test_property.py +++ b/tests/test_property.py @@ -247,5 +247,7 @@ def set_prop(self, val: bool) -> None: """A setter named differently.""" pass + assert isinstance(Example.prop, tp.FunctionalProperty) assert Example.prop.name == "prop" - assert Example.prop is Example.set_prop + assert not isinstance(Example.set_prop, tp.FunctionalProperty) + assert callable(Example.set_prop) From 55dcb005e97b489c2097012ada8e6530aa7cba24 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 4 Aug 2025 11:02:14 +0100 Subject: [PATCH 44/59] Eliminate DocstringToMessage in errors --- src/labthings_fastapi/exceptions.py | 31 ++----------------------- src/labthings_fastapi/thing_property.py | 9 ++++--- 2 files changed, 6 insertions(+), 34 deletions(-) diff --git a/src/labthings_fastapi/exceptions.py b/src/labthings_fastapi/exceptions.py index 3e050546..1a6a03cd 100644 --- a/src/labthings_fastapi/exceptions.py +++ b/src/labthings_fastapi/exceptions.py @@ -1,7 +1,5 @@ """A submodule for custom LabThings-FastAPI Exceptions.""" -import inspect - # The "import x as x" syntax means symbols are interpreted as being re-exported, # so they won't be flagged as unused by the linter. # An __all__ for this module is less than helpful, unless we have an @@ -11,32 +9,7 @@ ) -class DocstringToMessage: - """A mixin to put Exception docstrings in as their default message.""" - - append_to_message: bool = True - - def __init__(self, message: str | None = None): - """Initialise an error with a message or its docstring. - - :param message: the optional message. - """ - # We ignore call-arg within this function because mypy can't know - # that this is a mixin, and super() will be an exception (which does - # accept a string argument to `__init__`). - doc = inspect.cleandoc(self.__doc__) if self.__doc__ else None - if message: - if doc and self.append_to_message: - super().__init__(message + "\n\n" + doc) # type: ignore[call-arg] - else: - super().__init__(message) # type: ignore[call-arg] - elif doc: - super().__init__(doc) # type: ignore[call-arg] - else: - super().__init__() - - -class NotConnectedToServerError(DocstringToMessage, RuntimeError): +class NotConnectedToServerError(RuntimeError): """The Thing is not connected to a server. This exception is called if an Action is called or @@ -46,7 +19,7 @@ class NotConnectedToServerError(DocstringToMessage, RuntimeError): """ -class ReadOnlyPropertyError(DocstringToMessage, AttributeError): +class ReadOnlyPropertyError(AttributeError): """A property is read-only. No setter has been defined for this `.FunctionalProperty`, so diff --git a/src/labthings_fastapi/thing_property.py b/src/labthings_fastapi/thing_property.py index a16ddd3b..37682dc5 100644 --- a/src/labthings_fastapi/thing_property.py +++ b/src/labthings_fastapi/thing_property.py @@ -75,7 +75,6 @@ class attribute. Documentation is in strings immediately following the from .utilities.introspection import return_type from .base_descriptor import BaseDescriptor from .exceptions import ( - DocstringToMessage, NotConnectedToServerError, ReadOnlyPropertyError, ) @@ -97,7 +96,7 @@ class attribute. Documentation is in strings immediately following the # The following exceptions are raised only when creating/setting up properties. -class OverspecifiedDefaultError(DocstringToMessage, ValueError): +class OverspecifiedDefaultError(ValueError): """The default value has been specified more than once. This error is raised when a `.DataProperty` is instantiated with both a @@ -105,7 +104,7 @@ class OverspecifiedDefaultError(DocstringToMessage, ValueError): """ -class MissingDefaultError(DocstringToMessage, ValueError): +class MissingDefaultError(ValueError): """The default value has not been specified. This error is raised when a `.DataProperty` is instantiated without a @@ -113,7 +112,7 @@ class MissingDefaultError(DocstringToMessage, ValueError): """ -class InconsistentTypeError(DocstringToMessage, TypeError): +class InconsistentTypeError(TypeError): """Different type hints have been given for a property. Every property should have a type hint, which may be provided in a few @@ -122,7 +121,7 @@ class InconsistentTypeError(DocstringToMessage, TypeError): """ -class MissingTypeError(DocstringToMessage, TypeError): +class MissingTypeError(TypeError): """No type hints have been given for a property. Every property should have a type hint, which may be provided in a few From df8c5a9dfea5f7adb593c5eb9073a093d6cc46cd Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 4 Aug 2025 11:22:00 +0100 Subject: [PATCH 45/59] Better documentation of type test. Thanks @bprobert97. --- typing_tests/README.md | 2 +- typing_tests/thing_definitions.py | 36 ++++++++++++++++++++++++++----- 2 files changed, 32 insertions(+), 6 deletions(-) diff --git a/typing_tests/README.md b/typing_tests/README.md index 6fd3117d..109d79cc 100644 --- a/typing_tests/README.md +++ b/typing_tests/README.md @@ -3,7 +3,7 @@ The codebase is type-checked with `mypy src/` and tested with `pytest`, however neither of these explicitly check that `mypy` can infer the correct types for `Thing` attributes like properties and actions. The Python files in this folder are intended to be checked using: ```terminal -mypy --warn-unused-ignores .\typing_tests\ +mypy --warn-unused-ignores typing_tests ``` The files include valid code that's accompanied by `assert_type` statements (which check the inferred types are what we expect them to be) as well as invalid code where the expected `mypy` errors are ignored. This tests for expected errors - if an expected error is not thrown, it will cause an `unused-ignore` error. diff --git a/typing_tests/thing_definitions.py b/typing_tests/thing_definitions.py index 516cfbeb..54be773a 100644 --- a/typing_tests/thing_definitions.py +++ b/typing_tests/thing_definitions.py @@ -3,7 +3,7 @@ This module checks that code defining a Thing may be type checked using mypy. -See README.md for more details. +See README.md for how it's run. """ import labthings_fastapi as lt @@ -27,12 +27,19 @@ def int_factory() -> int: """A property that is not bound to a Thing. This will go wrong at runtime if we access its ``model`` but it should -have its type inferred as an `int`. +have its type inferred as an `int`. This is intended to let mypy check +the default is of the correct type when used with dataclass-style syntax +(``prop: int = lt.property(default=0)`` ). """ assert_type(unbound_prop, int) unbound_prop_2 = lt.property(default_factory=int_factory) -"""A property that is not bound to a Thing, with a factory.""" +"""A property that is not bound to a Thing, with a factory. + +As with `.unbound_prop` this won't work at runtime, but its type should +be inferred as `int` (which allows checking the default type matches +the attribute type annotation, when used on a class). +""" assert_type(unbound_prop_2, int) @@ -73,7 +80,16 @@ class TestPropertyDefaultsMatch(lt.Thing): "This property should cause mypy to throw an error, as it has no default." listprop: list[int] = lt.property(default_factory=list) - "A list property with a default factory. Note the default factory is a less specific type." + """A list property with a default factory. + + Note the default factory is a less specific type. + + Default types must be compatible with the attribute type, but not + necessarily the same. This tests a common scenario, where the default (an + empty list) is compatible, but not the same as ``list[int]`` . + + Note this is "tested" simply by the absence of `mypy` errors. + """ # Check that the type hints on an instance of the class are correct. @@ -84,6 +100,7 @@ class TestPropertyDefaultsMatch(lt.Thing): assert_type(test_defaults_match.optionalintprop, int | None) assert_type(test_defaults_match.optionalintprop2, int | None) assert_type(test_defaults_match.optionalintprop3, int | None) +assert_type(test_defaults_match.optionalintprop4, int | None) # NB the types of the class attributes will be the same as the instance attributes # because of the type hint on `lt.property`. This is incorrect (the class attributes @@ -93,7 +110,16 @@ class TestPropertyDefaultsMatch(lt.Thing): class TestExplicitDescriptor(lt.Thing): - """A Thing that checks our explicit descriptor type hints are working.""" + r"""A Thing that checks our explicit descriptor type hints are working. + + This tests `.DataProperty` descriptors work as intended when used directly, + rather than via ``lt.property``\ . + + ``lt.property`` has a "white lie" on its return type, which makes it + work with dataclass-style syntax (type annotation on the class attribute + rather than part of the descriptor). It's therefore useful to test + the underlying class as well. + """ intprop1 = lt.DataProperty[int](default=0) """A DataProperty that should not cause mypy errors.""" From b2f597eb7b705a9142fa3b7aebad18b68116b351 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 4 Aug 2025 11:25:13 +0100 Subject: [PATCH 46/59] Remove unused typevar --- typing_tests/thing_definitions.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/typing_tests/thing_definitions.py b/typing_tests/thing_definitions.py index 54be773a..f6555aab 100644 --- a/typing_tests/thing_definitions.py +++ b/typing_tests/thing_definitions.py @@ -171,8 +171,8 @@ class TestExplicitDescriptor(lt.Thing): assert_type(TestExplicitDescriptor.optionalintprop2, lt.DataProperty[int | None]) assert_type(TestExplicitDescriptor.optionalintprop3, lt.DataProperty[int | None]) + Val = typing.TypeVar("Val") -Thing = typing.TypeVar("Thing", bound=lt.Thing) def f_property(getter: typing.Callable[..., Val]) -> FunctionalProperty[Val]: From f976ac1d1aa6a4d95c2ad7c399568fae4a2aea5f Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 4 Aug 2025 11:29:59 +0100 Subject: [PATCH 47/59] Better comments/docstrings --- typing_tests/thing_definitions.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/typing_tests/thing_definitions.py b/typing_tests/thing_definitions.py index f6555aab..fe997a9a 100644 --- a/typing_tests/thing_definitions.py +++ b/typing_tests/thing_definitions.py @@ -190,7 +190,12 @@ def intprop1(self) -> int: @lt.property def intprop2(self) -> int: - """This property should not cause mypy errors and is writeable.""" + """This property should not cause mypy errors and is writeable. + + This property has a getter and setter, so it can be read and written + from other code within the Thing. However, we make it read-only for + client code (over HTTP or a DirectThingClient). + """ return 0 @intprop2.setter @@ -198,7 +203,10 @@ def set_intprop2(self, value: int): """Setter for intprop2.""" pass - intprop2.readonly = True # This should be an OK thing to do. + # Make the property read-only in the Thing Description and for HTTP + # clients, or DirectThingClients. See property documentation on + # readthedocs for examples. + intprop2.readonly = True @lt.property def intprop3(self) -> int: From 9ed747ad8917787f81ae8bb5f22286691567aefd Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 4 Aug 2025 23:32:16 +0100 Subject: [PATCH 48/59] Add more detailed tests of settings This has uncovered a bug in DirectThingClient (#165). For now, the test reflects current behaviour, but it will need an update when #165 is fixed. --- tests/__init__.py | 0 tests/test_settings.py | 181 +++++++++++++++++++++++++++++++++++++---- 2 files changed, 163 insertions(+), 18 deletions(-) create mode 100644 tests/__init__.py diff --git a/tests/__init__.py b/tests/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/tests/test_settings.py b/tests/test_settings.py index 17b55477..5e8d33dc 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -8,6 +8,7 @@ from fastapi.testclient import TestClient import labthings_fastapi as lt +from .temp_client import poll_task class TestThing(lt.Thing): @@ -17,6 +18,7 @@ def __init__(self) -> None: super().__init__() # Initialize functional settings with default values self._floatsetting: float = 1.0 + self._localonlysetting = "Local-only default." boolsetting: bool = lt.setting(default=False) "A boolean setting" @@ -37,15 +39,42 @@ def floatsetting(self, value: float): self._floatsetting = value @lt.setting - def readonlysetting(self) -> str: - """A read-only setting.""" - return "This is read-only" + def localonlysetting(self) -> str: + """A setting that is not writeable from HTTP clients or DirectThingClients. - @readonlysetting.setter - def readonlysetting(self, value: str): - pass + This setting has a setter, so may be written to from this Thing, or + when settings are loaded. However, it's marked as read-only later, which + means HTTP clients or DirectThingClient subclasses can't write to it. + """ + return self._localonlysetting - readonlysetting.readonly = True + @localonlysetting.setter + def localonlysetting(self, value: str): + self._localonlysetting = value + + localonlysetting.readonly = True + + localonly_boolsetting: bool = lt.setting(default=False, readonly=True) + + @lt.thing_action + def write_localonly_setting(self, value: str) -> None: + """Change the value of the local-only setting. + + This is allowed - the setting is only read-only for code running + over HTTP or via a DirectThingClient. By using this action, we can + check it's writeable for local code. + """ + self.localonlysetting = value + + @lt.thing_action + def toggle_localonly_boolsetting(self) -> None: + """Toggle the local-only bool setting. + + Settings with `readonly=True` are read-only for client code via HTTP + or a DirectThingClient. This action checks they are still writeable + from within the Thing. + """ + self.localonly_boolsetting = not self.localonly_boolsetting @lt.thing_action def toggle_boolsetting(self): @@ -57,13 +86,66 @@ def toggle_boolsetting_from_thread(self): t.start() +TestThingClientDep = lt.deps.direct_thing_client_dependency(TestThing, "/thing/") +TestThingDep = lt.deps.raw_thing_dependency(TestThing) + + +class ClientThing(lt.Thing): + """This Thing attempts to set read-only settings on TestThing. + + Read-only settings may not be set by DirectThingClient wrappers, + which is what this class tests. + """ + + @lt.thing_action + def set_localonlysetting( + self, + client: TestThingClientDep, + val: str, + ): + """Attempt to set a setting with a DirectThingClient.""" + client.localonlysetting = val + + @lt.thing_action + def set_localonly_boolsetting( + self, + client: TestThingClientDep, + val: bool, + ): + """Attempt to set a setting with a DirectThingClient.""" + client.localonly_boolsetting = val + + @lt.thing_action + def directly_set_localonlysetting( + self, + test_thing: TestThingDep, + val: str, + ): + """Attempt to set a setting directly.""" + test_thing.localonlysetting = val + + @lt.thing_action + def directly_set_localonly_boolsetting( + self, + test_thing: TestThingDep, + val: bool, + ): + """Attempt to set a setting directly.""" + test_thing.localonly_boolsetting = val + + def _get_setting_file(server, thingpath): path = os.path.join(server.settings_folder, thingpath.lstrip("/"), "settings.json") return os.path.normpath(path) def _settings_dict( - boolsetting=False, floatsetting=1.0, stringsetting="foo", dictsetting=None + boolsetting=False, + floatsetting=1.0, + stringsetting="foo", + dictsetting=None, + localonlysetting="Local-only default.", + localonly_boolsetting=False, ): """Return the expected settings dictionary @@ -76,7 +158,8 @@ def _settings_dict( "floatsetting": floatsetting, "stringsetting": stringsetting, "dictsetting": dictsetting, - "readonlysetting": "This is read-only", + "localonlysetting": localonlysetting, + "localonly_boolsetting": localonly_boolsetting, } @@ -85,6 +168,11 @@ def thing(): return TestThing() +@pytest.fixture +def client_thing(): + return ClientThing() + + @pytest.fixture def server(): with tempfile.TemporaryDirectory() as tempdir: @@ -98,7 +186,7 @@ def test_setting_available(thing): assert not thing.boolsetting assert thing.stringsetting == "foo" assert thing.floatsetting == 1.0 - assert thing.readonlysetting == "This is read-only" + assert thing.localonlysetting == "Local-only default." def test_functional_settings_save(thing, server): @@ -153,20 +241,77 @@ def test_data_settings_save(thing, server): assert json.load(file_obj) == _settings_dict(boolsetting=True) -def test_readonly_setting(thing, server): - """Check read-only settings cannot be set remotely.""" +@pytest.mark.parametrize( + ("endpoint", "value"), + [ + ("localonlysetting", "Other value"), + ("localonly_boolsetting", True), + ], +) +@pytest.mark.parametrize( + "method", + ["http", "direct_thing_client", "direct"], +) +def test_readonly_setting(thing, client_thing, server, endpoint, value, method): + """Check read-only functional settings cannot be set remotely. + + Functional settings must always have a setter, and will be + writeable from within the Thing. However, they should not + be settable remotely or via a DirectThingClient. + + This test is a bit complicated, but it checks both a + `.FunctionalSetting` and a `.DataSetting` via all three + methods: HTTP, DirectThingClient, and directly on the Thing. + Only the last method should work. + + The test is parametrized so it will run 6 times, trying one + block of code inside the ``with`` block each time. + """ setting_file = _get_setting_file(server, "/thing") server.add_thing(thing, "/thing") + server.add_thing(client_thing, "/client_thing") # No setting file created when first added assert not os.path.isfile(setting_file) + + # Access it over "HTTP" with a TestClient + # This doesn't actually serve over the network but will use + # all the same codepaths. with TestClient(server.app) as client: - # Check we can read it over HTTP - r = client.get("/thing/readonlysetting") - assert r.json() == "This is read-only" + if method == "http": + # Attempt to set read-only setting + r = client.put(f"/thing/{endpoint}", json=value) + assert r.status_code == 405 + + if method == "direct_thing_client": + # Attempt to set read-only setting via a DirectThingClient + r = client.post(f"/client_thing/set_{endpoint}", json={"val": value}) + assert r.status_code == 201 + invocation = poll_task(client, r.json()) + # The setting is not changed (that's tested later), but the action + # does complete. It should fail with an error, but this is expected + # behaviour - see #165. + assert invocation["status"] == "completed" + + # Check the setting hasn't changed over HTTP + r = client.get(f"/thing/{endpoint}") + assert r.json() == _settings_dict()[endpoint] assert r.status_code == 200 - # Attempt to set read-only setting - r = client.put("/thing/readonlysetting", json="new value") - assert r.status_code == 405 + + if method == "direct": + # Actually set read-only setting via raw_thing_dependency + r = client.post( + f"/client_thing/directly_set_{endpoint}", json={"val": value} + ) + invocation = poll_task(client, r.json()) + assert invocation["status"] == "completed" + + if method == "direct": + # Setting directly should succeed, so the file should exist. + with open(setting_file, "r", encoding="utf-8") as file_obj: + # Check settings on file match expected dictionary + assert json.load(file_obj) == _settings_dict(**{endpoint: value}) + else: + # Other methods fail, so there should be no file here. assert not os.path.isfile(setting_file) # No file created From 309aa8abcec49a9d56f8b6c5b4a958a5fafca6ee Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 4 Aug 2025 23:40:01 +0100 Subject: [PATCH 49/59] Add a test for dictsetting --- tests/test_settings.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_settings.py b/tests/test_settings.py index 5e8d33dc..8183149a 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -187,6 +187,7 @@ def test_setting_available(thing): assert thing.stringsetting == "foo" assert thing.floatsetting == 1.0 assert thing.localonlysetting == "Local-only default." + assert thing.dictsetting == {"a": 1, "b": 2} def test_functional_settings_save(thing, server): From f30f4253ab35bdc0ed0f41e8ddeec3b492b57e64 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 4 Aug 2025 23:42:32 +0100 Subject: [PATCH 50/59] Add comment on cache --- src/labthings_fastapi/base_descriptor.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/labthings_fastapi/base_descriptor.py b/src/labthings_fastapi/base_descriptor.py index 374477cc..919a1f0d 100644 --- a/src/labthings_fastapi/base_descriptor.py +++ b/src/labthings_fastapi/base_descriptor.py @@ -348,6 +348,13 @@ def instance_get(self, obj: Thing) -> Value: ) +# get_class_attribute_docstrings is a relatively expensive function that +# will be called potentially quite a few times on the same class. It will +# return the same result each time (because it depends only on the source +# code of the class, which can't change), so it makes sense to cache it. +# +# We use weak keys to avoid messing up garbage collection, and cache the +# mapping of attribute names to attribute docstrings. _class_attribute_docstring_cache: WeakKeyDictionary[type, Mapping[str, str]] = ( WeakKeyDictionary() ) From f3963dffb02d8af27444eac949febb0ff9a30c17 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 4 Aug 2025 23:45:31 +0100 Subject: [PATCH 51/59] Fix docstring format --- tests/test_property.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_property.py b/tests/test_property.py index 28538d78..b4ff5702 100644 --- a/tests/test_property.py +++ b/tests/test_property.py @@ -20,9 +20,9 @@ def test_default_factory_from_arguments(): - """Check the function that implements default/default_factory + """Check the function that implements default/default_factory behaves correctly. - behaves correctly. It should always return a function that + It should always return a function that returns a default value, and should error if both arguments are provided, or if none are provided. """ From 4cd98735baa03ba0ffe8014588c845c3a0c1f452 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Tue, 5 Aug 2025 11:16:15 +0100 Subject: [PATCH 52/59] Fix spelling errors --- src/labthings_fastapi/thing_property.py | 2 +- tests/test_base_descriptor.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/labthings_fastapi/thing_property.py b/src/labthings_fastapi/thing_property.py index 37682dc5..ac8a9313 100644 --- a/src/labthings_fastapi/thing_property.py +++ b/src/labthings_fastapi/thing_property.py @@ -768,7 +768,7 @@ def set_myprop(self, val: int) -> None: function. It will cause problems elsewhere in the code if descriptors are assigned - to more than one attrubute, and this is checked in + to more than one attribute, and this is checked in `.BaseDescriptor.__set_name__`\ . We therefore return the setter rather than the descriptor if the names don't match. The type hint does not reflect this, as it would cause problems when the names do match (the diff --git a/tests/test_base_descriptor.py b/tests/test_base_descriptor.py index 242a8f4a..0f8d1cd8 100644 --- a/tests/test_base_descriptor.py +++ b/tests/test_base_descriptor.py @@ -270,7 +270,7 @@ def set_prop1(self, val): assert "prop1" in str(excinfo.value) assert "set_prop1" in str(excinfo.value) - # For good measure, check re-use across classes is also prevented. + # For good measure, check reuse across classes is also prevented. class FirstExampleClass: prop = BaseDescriptor() From 7f4e8dd8a64c983f015847da42fd34768b388576 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Tue, 5 Aug 2025 11:35:47 +0100 Subject: [PATCH 53/59] Fix test imports I added an __init__.py to the tests folder in order to nicely import `poll_task`. This means I need to change all the other imports of test code to use relative imports starting with `.`. --- tests/test_action_cancel.py | 2 +- tests/test_action_logging.py | 2 +- tests/test_action_manager.py | 2 +- tests/test_actions.py | 2 +- tests/test_dependencies.py | 2 +- tests/test_dependencies_2.py | 2 +- tests/test_dependency_metadata.py | 2 +- tests/test_docs.py | 2 +- tests/test_properties.py | 2 +- tests/test_thing_dependencies.py | 2 +- 10 files changed, 10 insertions(+), 10 deletions(-) diff --git a/tests/test_action_cancel.py b/tests/test_action_cancel.py index ead108e7..7ec923cf 100644 --- a/tests/test_action_cancel.py +++ b/tests/test_action_cancel.py @@ -4,7 +4,7 @@ import uuid from fastapi.testclient import TestClient -from temp_client import poll_task, task_href +from .temp_client import poll_task, task_href import labthings_fastapi as lt import time diff --git a/tests/test_action_logging.py b/tests/test_action_logging.py index c98c08b0..86fecf9e 100644 --- a/tests/test_action_logging.py +++ b/tests/test_action_logging.py @@ -4,7 +4,7 @@ import logging from fastapi.testclient import TestClient -from temp_client import poll_task +from .temp_client import poll_task import labthings_fastapi as lt from labthings_fastapi.actions.invocation_model import LogRecordModel diff --git a/tests/test_action_manager.py b/tests/test_action_manager.py index c38b50d7..eea32c9a 100644 --- a/tests/test_action_manager.py +++ b/tests/test_action_manager.py @@ -1,7 +1,7 @@ from fastapi.testclient import TestClient import pytest import httpx -from temp_client import poll_task +from .temp_client import poll_task import time import labthings_fastapi as lt from labthings_fastapi.actions import ACTION_INVOCATIONS_PATH diff --git a/tests/test_actions.py b/tests/test_actions.py index 18a5d257..98c6949d 100644 --- a/tests/test_actions.py +++ b/tests/test_actions.py @@ -1,6 +1,6 @@ from fastapi.testclient import TestClient import pytest -from temp_client import poll_task, get_link +from .temp_client import poll_task, get_link from labthings_fastapi.example_things import MyThing import labthings_fastapi as lt diff --git a/tests/test_dependencies.py b/tests/test_dependencies.py index 2d6f4689..a8195d86 100644 --- a/tests/test_dependencies.py +++ b/tests/test_dependencies.py @@ -7,7 +7,7 @@ from fastapi import Depends, FastAPI, Request from labthings_fastapi.deps import InvocationID from fastapi.testclient import TestClient -from module_with_deps import FancyIDDep +from .module_with_deps import FancyIDDep def test_invocation_id(): diff --git a/tests/test_dependencies_2.py b/tests/test_dependencies_2.py index cf5a2a33..8702ead0 100644 --- a/tests/test_dependencies_2.py +++ b/tests/test_dependencies_2.py @@ -20,7 +20,7 @@ from typing import Annotated from fastapi import Depends, FastAPI from fastapi.testclient import TestClient -from module_with_deps import FancyIDDep, FancyID, ClassDependsOnFancyID +from .module_with_deps import FancyIDDep, FancyID, ClassDependsOnFancyID import labthings_fastapi as lt diff --git a/tests/test_dependency_metadata.py b/tests/test_dependency_metadata.py index d6449438..c0a04e7d 100644 --- a/tests/test_dependency_metadata.py +++ b/tests/test_dependency_metadata.py @@ -4,7 +4,7 @@ from typing import Any, Mapping from fastapi.testclient import TestClient -from temp_client import poll_task +from .temp_client import poll_task import labthings_fastapi as lt diff --git a/tests/test_docs.py b/tests/test_docs.py index 954b1068..7b705bc3 100644 --- a/tests/test_docs.py +++ b/tests/test_docs.py @@ -1,8 +1,8 @@ from pathlib import Path from runpy import run_path -from test_server_cli import MonitoredProcess from fastapi.testclient import TestClient from labthings_fastapi import ThingClient +from .test_server_cli import MonitoredProcess this_file = Path(__file__) diff --git a/tests/test_properties.py b/tests/test_properties.py index 9fe11802..9380798e 100644 --- a/tests/test_properties.py +++ b/tests/test_properties.py @@ -8,7 +8,7 @@ import labthings_fastapi as lt from labthings_fastapi.exceptions import NotConnectedToServerError -from temp_client import poll_task +from .temp_client import poll_task class PropertyTestThing(lt.Thing): diff --git a/tests/test_thing_dependencies.py b/tests/test_thing_dependencies.py index 1279cd37..a9fd7a30 100644 --- a/tests/test_thing_dependencies.py +++ b/tests/test_thing_dependencies.py @@ -7,7 +7,7 @@ from fastapi import Request import pytest import labthings_fastapi as lt -from temp_client import poll_task +from .temp_client import poll_task from labthings_fastapi.client.in_server import direct_thing_client_class from labthings_fastapi.utilities.introspection import fastapi_dependency_params From f9885660c9c91987ed922688018de0a9d9e8b437 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Tue, 5 Aug 2025 12:10:13 +0100 Subject: [PATCH 54/59] Fix tests of basedescriptor errors on old Python versions. See explanation in `raises_or_is_caused_by` in `test_base_descriptor`. --- tests/test_base_descriptor.py | 35 ++++++++++++++++++++++++++++++++--- 1 file changed, 32 insertions(+), 3 deletions(-) diff --git a/tests/test_base_descriptor.py b/tests/test_base_descriptor.py index 0f8d1cd8..1121357f 100644 --- a/tests/test_base_descriptor.py +++ b/tests/test_base_descriptor.py @@ -1,3 +1,5 @@ +from contextlib import contextmanager +from typing import Iterator import pytest from labthings_fastapi.base_descriptor import ( BaseDescriptor, @@ -211,6 +213,33 @@ def __set_name__(self, owner, name): super().__set_name__(owner, name) +@contextmanager +def raises_or_is_caused_by( + exception_cls: type[Exception], +) -> Iterator[pytest.ExceptionInfo]: + r"""Wrap `pytest.raises` to cope with exceptions that are wrapped in another error. + + Some errors raised during class creation are wrapped in a `RuntimeError` on older + Python versions. This makes them harder to test for. + + This context manager checks the exception, and if it is not the expected class it + will then check the ``__cause__`` attribute. If the ``__cause__`` matches, we + replace the exception in the yielded ``excinfo`` object with its ``__cause__`` + so that the correct exception may be inspected. + + If neither matches, we will fail with an `AssertionError`\ . + """ + with pytest.raises(Exception) as excinfo: + yield excinfo + if not isinstance(excinfo.value, exception_cls): + assert isinstance(excinfo.value.__cause__, exception_cls) + assert excinfo._excinfo is not None + # If excinfo._excinfo is None, we missed an exception and the code should + # already have failed. + traceback = excinfo._excinfo[2] + excinfo._excinfo = (exception_cls, excinfo.value.__cause__, traceback) + + def test_decorator_different_names(): """Check that adding a descriptor to a class twice raises the right error. @@ -222,7 +251,7 @@ def test_decorator_different_names(): exception is tested in ``test_property.py`` in this folder. """ # First, very obviously double-assign a BaseDescriptor - with pytest.raises(DescriptorAddedToClassTwiceError) as excinfo: + with raises_or_is_caused_by(DescriptorAddedToClassTwiceError) as excinfo: class ExplicitExample: """An example class.""" @@ -252,7 +281,7 @@ def prop1(self, val): # It should raise an error here, but is a special case in # `.FunctionalProperty.__set_name__` so will be OK for `.FunctionalProperty` # and `.FunctionalSetting` as a result. - with pytest.raises(DescriptorAddedToClassTwiceError) as excinfo: + with raises_or_is_caused_by(DescriptorAddedToClassTwiceError) as excinfo: class DecoratorExample: """Another example class.""" @@ -274,7 +303,7 @@ def set_prop1(self, val): class FirstExampleClass: prop = BaseDescriptor() - with pytest.raises(DescriptorAddedToClassTwiceError) as excinfo: + with raises_or_is_caused_by(DescriptorAddedToClassTwiceError) as excinfo: class SecondExampleClass: prop = FirstExampleClass.prop From c092f9c8903b8c4df707b45fb179619cd6f5b658 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Tue, 5 Aug 2025 12:14:02 +0100 Subject: [PATCH 55/59] Use raises_or_is_caused_by in test_properties I've moved this useful function into a utilities module. --- tests/test_base_descriptor.py | 30 +----------------------------- tests/test_property.py | 3 ++- tests/utilities.py | 32 ++++++++++++++++++++++++++++++++ 3 files changed, 35 insertions(+), 30 deletions(-) create mode 100644 tests/utilities.py diff --git a/tests/test_base_descriptor.py b/tests/test_base_descriptor.py index 1121357f..e283d438 100644 --- a/tests/test_base_descriptor.py +++ b/tests/test_base_descriptor.py @@ -1,5 +1,3 @@ -from contextlib import contextmanager -from typing import Iterator import pytest from labthings_fastapi.base_descriptor import ( BaseDescriptor, @@ -7,6 +5,7 @@ DescriptorAddedToClassTwiceError, get_class_attribute_docstrings, ) +from .utilities import raises_or_is_caused_by class MockProperty(BaseDescriptor[str]): @@ -213,33 +212,6 @@ def __set_name__(self, owner, name): super().__set_name__(owner, name) -@contextmanager -def raises_or_is_caused_by( - exception_cls: type[Exception], -) -> Iterator[pytest.ExceptionInfo]: - r"""Wrap `pytest.raises` to cope with exceptions that are wrapped in another error. - - Some errors raised during class creation are wrapped in a `RuntimeError` on older - Python versions. This makes them harder to test for. - - This context manager checks the exception, and if it is not the expected class it - will then check the ``__cause__`` attribute. If the ``__cause__`` matches, we - replace the exception in the yielded ``excinfo`` object with its ``__cause__`` - so that the correct exception may be inspected. - - If neither matches, we will fail with an `AssertionError`\ . - """ - with pytest.raises(Exception) as excinfo: - yield excinfo - if not isinstance(excinfo.value, exception_cls): - assert isinstance(excinfo.value.__cause__, exception_cls) - assert excinfo._excinfo is not None - # If excinfo._excinfo is None, we missed an exception and the code should - # already have failed. - traceback = excinfo._excinfo[2] - excinfo._excinfo = (exception_cls, excinfo.value.__cause__, traceback) - - def test_decorator_different_names(): """Check that adding a descriptor to a class twice raises the right error. diff --git a/tests/test_property.py b/tests/test_property.py index b4ff5702..65867a8e 100644 --- a/tests/test_property.py +++ b/tests/test_property.py @@ -17,6 +17,7 @@ import pytest from labthings_fastapi import thing_property as tp from labthings_fastapi.base_descriptor import DescriptorAddedToClassTwiceError +from .utilities import raises_or_is_caused_by def test_default_factory_from_arguments(): @@ -228,7 +229,7 @@ def test_decorator_exception(): """ # The exception should be specific - a simple double assignment is # still an error - with pytest.raises(DescriptorAddedToClassTwiceError): + with raises_or_is_caused_by(DescriptorAddedToClassTwiceError): class BadExample: """A class with a wrongly reused descriptor.""" diff --git a/tests/utilities.py b/tests/utilities.py new file mode 100644 index 00000000..c5d3a574 --- /dev/null +++ b/tests/utilities.py @@ -0,0 +1,32 @@ +"""Useful functions for test code.""" + +from contextlib import contextmanager +from typing import Iterator +import pytest + + +@contextmanager +def raises_or_is_caused_by( + exception_cls: type[Exception], +) -> Iterator[pytest.ExceptionInfo]: + r"""Wrap `pytest.raises` to cope with exceptions that are wrapped in another error. + + Some errors raised during class creation are wrapped in a `RuntimeError` on older + Python versions. This makes them harder to test for. + + This context manager checks the exception, and if it is not the expected class it + will then check the ``__cause__`` attribute. If the ``__cause__`` matches, we + replace the exception in the yielded ``excinfo`` object with its ``__cause__`` + so that the correct exception may be inspected. + + If neither matches, we will fail with an `AssertionError`\ . + """ + with pytest.raises(Exception) as excinfo: + yield excinfo + if not isinstance(excinfo.value, exception_cls): + assert isinstance(excinfo.value.__cause__, exception_cls) + assert excinfo._excinfo is not None + # If excinfo._excinfo is None, we missed an exception and the code should + # already have failed. + traceback = excinfo._excinfo[2] + excinfo._excinfo = (exception_cls, excinfo.value.__cause__, traceback) From 124d96364b01caa4949a9e800317d3e651e0a4d7 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Fri, 8 Aug 2025 23:00:48 +0100 Subject: [PATCH 56/59] Added a test for redefinition errors when getter/setter names match. I've also added a few more comments to the typing test, explaining what some of the lines are checking for. --- typing_tests/thing_definitions.py | 45 ++++++++++++++++++++++++++++--- 1 file changed, 41 insertions(+), 4 deletions(-) diff --git a/typing_tests/thing_definitions.py b/typing_tests/thing_definitions.py index fe997a9a..10f8c281 100644 --- a/typing_tests/thing_definitions.py +++ b/typing_tests/thing_definitions.py @@ -3,6 +3,20 @@ This module checks that code defining a Thing may be type checked using mypy. +Note that most of the properties are typed as ``int`` or ``int | None`` +and we do not attempt to cover all possible types. A greater range of types +should be tested in code that's actually run, in the `tests` folder. For +this file, what's important is checking that: + +1. The type of the default/factory is compatible with the property + (though not necessarily identical). +2. Errors are raised if types don't match. +3. Class and instance attributes have the expected types. + +This requires at least a couple of types, where one is compatible +with the other, hence ``int`` and ``int | None`` which lets us +check compatibility, and also check ``None`` is OK as a default. + See README.md for how it's run. """ @@ -176,7 +190,7 @@ class TestExplicitDescriptor(lt.Thing): def f_property(getter: typing.Callable[..., Val]) -> FunctionalProperty[Val]: - """A function that returns a DataProperty with a getter.""" + """A function that returns a FunctionalProperty with a getter.""" return FunctionalProperty(getter) @@ -220,12 +234,32 @@ def set_intprop3(self, value: str) -> None: @f_property def fprop(self) -> int: - """A functional property that should not cause mypy errors.""" + """A functional property that should not cause mypy errors. + + This uses a much simpler function than ``lt.property`` to check + the behaviour is the same. + """ return 0 - @strprop.setter + @fprop.setter def set_fprop(self, value: int) -> None: - """Setter for strprop. This should fail type checking.""" + """Setter for fprop. Type checking should pass.""" + pass + + @lt.property + def strprop(self) -> str: + """A property with identically named getter/setter.""" + return "Hello world!" + + @strprop.setter # type: ignore[no-redef] + def strprop(self, val: str) -> None: + """A setter with the same name as the getter. + + This is the convention for `builtins.property` but `mypy` does not + allow it for any other property-like decorators. + + This function should raise a ``no-redef`` error. + """ pass @@ -233,9 +267,12 @@ def set_fprop(self, value: int) -> None: assert_type(TestFunctionalProperty.intprop2, FunctionalProperty[int]) assert_type(TestFunctionalProperty.intprop3, FunctionalProperty[int]) assert_type(TestFunctionalProperty.fprop, FunctionalProperty[int]) +# Don't check ``strprop`` because it caused an error and thus will +# not have the right type, even though the error is ignored. test_functional_property = TestFunctionalProperty() assert_type(test_functional_property.intprop1, int) assert_type(test_functional_property.intprop2, int) assert_type(test_functional_property.intprop3, int) assert_type(test_functional_property.fprop, int) +# ``strprop`` will be ``Any`` because of the ``[no-redef]`` error. From 316a775931e1c6a6084df077c0258b58dd13c676 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Fri, 8 Aug 2025 23:18:58 +0100 Subject: [PATCH 57/59] Get rid of unnecessary #noqa `pydocstyle` linter rules ("D" codes) were running in `flake8` and `ruff`. `ruff` is recommended by `pydocstyle` as having feature parity and being actively maintained. `flake8` didn't recognise `builtins.property` as a property and needed D401 ignored each time. I've removed `flake8-docstrings` (plugin for pydocstyle) and the associated #noqa statements. The relevant rules are still checked by `ruff`. I don't believe this has relaxed any linter rules, just deduplicated. --- pyproject.toml | 10 ++++++++-- src/labthings_fastapi/thing_property.py | 14 ++++++++------ 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 29bfe47c..772475e2 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -33,7 +33,6 @@ dev = [ "Pillow", "flake8", "flake8-pyproject", - "flake8-docstrings", "flake8-rst", "flake8-rst-docstrings", "pydoclint[flake8]", @@ -96,6 +95,14 @@ preview = true "typing_tests/*" = ["D404", "D401"] "docs/*" = ["D", "DOC"] +[tool.ruff.lint.pydocstyle] +# This lets the D401 checker understand that decorated thing properties and thing +# settings act like properties so should be documented as such. +property-decorators = [ + "labthings_fastapi.property", + "labthings_fastapi.setting", +] + [tool.mypy] plugins = ["pydantic.mypy"] @@ -103,7 +110,6 @@ plugins = ["pydantic.mypy"] [tool.flake8] extend-ignore = [ "DOC301", # allow class + __init__ docstrings - "D202", # conflicts with ruff format "F401", # already implemented by ruff, which respects "import x as x" "E301", # leave this to ruff (blank lines) "E302", # leave this to ruff (blank lines) diff --git a/src/labthings_fastapi/thing_property.py b/src/labthings_fastapi/thing_property.py index ac8a9313..9af1bbf9 100644 --- a/src/labthings_fastapi/thing_property.py +++ b/src/labthings_fastapi/thing_property.py @@ -90,9 +90,11 @@ class attribute. Documentation is in strings immediately following the # function they overload. # D105 is the same as D103, but for __init__ (i.e. magic methods). # DOC101 and DOC103 are also a result of overloads not having docstrings -# DOC201 and D401 are ignored on properties. Because we are overriding the +# DOC201 is ignored on properties. Because we are overriding the # builtin `property`, we are using `@builtins.property` which is not recognised # by pydoclint as a property. I've therefore ignored those codes manually. +# pydocstyle ("D" codes) is run in Ruff and correctly recognises +# builtins.property as a property decorator. # The following exceptions are raised only when creating/setting up properties. @@ -342,7 +344,7 @@ def value_type(self) -> type[Value]: :raises MissingTypeError: if the type has not been set. :return: the type of the descriptor's value. - """ # noqa: D401 # see note at the top of the file + """ if self._type is None: raise MissingTypeError("This property does not have a valid type.") return self._type @@ -360,7 +362,7 @@ def model(self) -> type[BaseModel]: subclass, this returns it unchanged. :return: a Pydantic model for the property's type. - """ # noqa: D401 # see note at the top of the file + """ if self._model is None: self._model = wrap_plain_types_in_rootmodel(self.value_type) return self._model @@ -567,7 +569,7 @@ class MyThing(Thing): @builtins.property def value_type(self) -> type[Value]: # noqa: DOC201 - """The type of the descriptor's value.""" # noqa: D401 + """The type of the descriptor's value.""" self.assert_set_name_called() return super().value_type @@ -690,12 +692,12 @@ def __init__( @builtins.property def fget(self) -> ValueGetter: # noqa: DOC201 - """The getter function.""" # noqa: D401 + """The getter function.""" return self._fget @builtins.property def fset(self) -> ValueSetter | None: # noqa: DOC201 - """The setter function.""" # noqa: D401 + """The setter function.""" return self._fset def getter(self, fget: ValueGetter) -> Self: From 8ce08ce383d3bcf269c50b8542cb227f8eacaff0 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Sat, 9 Aug 2025 07:01:35 +0100 Subject: [PATCH 58/59] Re-lock dependencies without flake8-docstrings --- dev-requirements.txt | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/dev-requirements.txt b/dev-requirements.txt index 27bf0221..c2a38e6b 100644 --- a/dev-requirements.txt +++ b/dev-requirements.txt @@ -68,13 +68,10 @@ fastapi-cloud-cli==0.1.4 flake8==7.3.0 # via # labthings-fastapi (pyproject.toml) - # flake8-docstrings # flake8-pyproject # flake8-rst # flake8-rst-docstrings # pydoclint -flake8-docstrings==1.7.0 - # via labthings-fastapi (pyproject.toml) flake8-pyproject==1.2.3 # via labthings-fastapi (pyproject.toml) flake8-rst==0.8.0 @@ -162,8 +159,6 @@ pydantic-settings==2.10.1 # via fastapi pydoclint==0.6.6 # via labthings-fastapi (pyproject.toml) -pydocstyle==6.3.0 - # via flake8-docstrings pyflakes==3.4.0 # via flake8 pygments==2.19.2 @@ -220,9 +215,7 @@ shellingham==1.5.4 sniffio==1.3.1 # via anyio snowballstemmer==3.0.1 - # via - # pydocstyle - # sphinx + # via sphinx sphinx==8.1.3 # via # labthings-fastapi (pyproject.toml) From 45de7369d8fdce8c060d0fdb8459d9535af7236c Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Tue, 12 Aug 2025 11:43:16 +0100 Subject: [PATCH 59/59] Rename `thing_property` to `properties` The module that includes `property` and `setting` is renamed. There are two main reasons for this: 1. The "thing" in the name is a bit redundant, and it matches "actions". 2. `lt.thing_property` is a symbol we've removed in this branch. Using it as a module name makes for confusing error messages. --- src/labthings_fastapi/__init__.py | 2 +- src/labthings_fastapi/client/in_server.py | 2 +- src/labthings_fastapi/example_things/__init__.py | 2 +- src/labthings_fastapi/{thing_property.py => properties.py} | 0 src/labthings_fastapi/thing.py | 2 +- tests/test_property.py | 2 +- typing_tests/thing_definitions.py | 2 +- 7 files changed, 6 insertions(+), 6 deletions(-) rename src/labthings_fastapi/{thing_property.py => properties.py} (100%) diff --git a/src/labthings_fastapi/__init__.py b/src/labthings_fastapi/__init__.py index 20da7f5a..a77559ef 100644 --- a/src/labthings_fastapi/__init__.py +++ b/src/labthings_fastapi/__init__.py @@ -20,7 +20,7 @@ """ from .thing import Thing -from .thing_property import property, setting, DataProperty, DataSetting +from .properties import property, setting, DataProperty, DataSetting from .decorators import ( thing_action, fastapi_endpoint, diff --git a/src/labthings_fastapi/client/in_server.py b/src/labthings_fastapi/client/in_server.py index f3880d12..2aedf6c6 100644 --- a/src/labthings_fastapi/client/in_server.py +++ b/src/labthings_fastapi/client/in_server.py @@ -21,7 +21,7 @@ from pydantic import BaseModel from ..descriptors.action import ActionDescriptor -from ..thing_property import BaseProperty +from ..properties import BaseProperty from ..utilities import attributes from . import PropertyClientDescriptor from ..thing import Thing diff --git a/src/labthings_fastapi/example_things/__init__.py b/src/labthings_fastapi/example_things/__init__.py index f26c1806..3ed50647 100644 --- a/src/labthings_fastapi/example_things/__init__.py +++ b/src/labthings_fastapi/example_things/__init__.py @@ -8,7 +8,7 @@ from typing import Any, Optional, Annotated from labthings_fastapi.thing import Thing from labthings_fastapi.decorators import thing_action -from labthings_fastapi.thing_property import property as lt_property +from labthings_fastapi.properties import property as lt_property from pydantic import Field diff --git a/src/labthings_fastapi/thing_property.py b/src/labthings_fastapi/properties.py similarity index 100% rename from src/labthings_fastapi/thing_property.py rename to src/labthings_fastapi/properties.py diff --git a/src/labthings_fastapi/thing.py b/src/labthings_fastapi/thing.py index fd03a0c3..3d3cb739 100644 --- a/src/labthings_fastapi/thing.py +++ b/src/labthings_fastapi/thing.py @@ -21,7 +21,7 @@ from pydantic import BaseModel -from .thing_property import DataProperty, BaseSetting +from .properties import DataProperty, BaseSetting from .descriptors import ActionDescriptor from .thing_description._model import ThingDescription, NoSecurityScheme from .utilities import class_attributes diff --git a/tests/test_property.py b/tests/test_property.py index 65867a8e..e15ab1be 100644 --- a/tests/test_property.py +++ b/tests/test_property.py @@ -15,7 +15,7 @@ from fastapi.testclient import TestClient import pydantic import pytest -from labthings_fastapi import thing_property as tp +from labthings_fastapi import properties as tp from labthings_fastapi.base_descriptor import DescriptorAddedToClassTwiceError from .utilities import raises_or_is_caused_by diff --git a/typing_tests/thing_definitions.py b/typing_tests/thing_definitions.py index 10f8c281..b0815e91 100644 --- a/typing_tests/thing_definitions.py +++ b/typing_tests/thing_definitions.py @@ -21,7 +21,7 @@ """ import labthings_fastapi as lt -from labthings_fastapi.thing_property import FunctionalProperty +from labthings_fastapi.properties import FunctionalProperty from typing_extensions import assert_type import typing