From 9a907d96ca56bd535ccfe561fc3a63b551e621d9 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 4 Sep 2026 00:02:44 +0000 Subject: [PATCH 1/4] attributes: @attr decorator sugar over the getter/setter constructors Adds the `@attr` decorator from ADR 0018: a controller declares an attribute by decorating the method that reads it, with `@x.setter` for the writer half, mirroring `@property`. The datatype comes from the getter's return annotation (unwrapping `Update[T]`), the getter's docstring summary becomes the description, and decorator keyword arguments are the attribute's metadata, validated against the datatype. The optional leading positional is a `Polled`/`NotPolled` schedule, so the declarative and procedural spellings share one vocabulary; a bare `@attr` is read once at connect, as a bare `getter=` is. Binding follows `@command`/`@scan`: the class body holds an `UnboundAttr` declaration and each controller instance binds a fresh `AttrR`/`AttrRW` of its own, so nothing is deepcopied from a class-scope prototype. `UnboundAttr` is a non-data descriptor so that a decorated attribute reads as the attribute it becomes rather than the declaration - `UnboundAttrRW` carries the `AttrRW` typing. Adds the "FastCS for PyTango users" docs page. Closes #397 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SiGhLM9QRKpnQykdmMfLsh --- docs/how-to/fastcs-for-pytango-users.md | 157 +++++++++ src/fastcs/attributes/__init__.py | 5 + src/fastcs/attributes/attr_decorator.py | 348 +++++++++++++++++++ src/fastcs/controllers/base_controller.py | 13 +- tests/test_attr_decorator.py | 404 ++++++++++++++++++++++ 5 files changed, 926 insertions(+), 1 deletion(-) create mode 100644 docs/how-to/fastcs-for-pytango-users.md create mode 100644 src/fastcs/attributes/attr_decorator.py create mode 100644 tests/test_attr_decorator.py diff --git a/docs/how-to/fastcs-for-pytango-users.md b/docs/how-to/fastcs-for-pytango-users.md new file mode 100644 index 00000000..1f82ac09 --- /dev/null +++ b/docs/how-to/fastcs-for-pytango-users.md @@ -0,0 +1,157 @@ +# FastCS for PyTango Users + +If you write Tango Device Servers with PyTango, the shape of a FastCS controller +will already be familiar: a class, some attributes, some commands. This page +pairs the PyTango spelling with the FastCS one, so you can carry what you know +across. + +The headline difference is that a FastCS controller is not tied to Tango. The +same class is served over Tango, EPICS (Channel Access or PV Access), REST and +GraphQL - see [](./multiple-transports.md). + +## Hello world + +PyTango's simplest attribute is one decorated getter: + +```python +from tango.server import Device, attribute + + +class PowerSupply(Device): + @attribute + def voltage(self) -> float: + return 2.5 +``` + +FastCS says the same thing with `@attr`: + +```python +from fastcs.attributes import attr +from fastcs.controllers import Controller + + +class PowerSupply(Controller): + @attr + async def voltage(self) -> float: + return 2.5 +``` + +Two differences to notice: + +- The getter is `async`. FastCS controllers run on one event loop, so a getter + that talks to a device awaits it rather than blocking every other attribute. +- The datatype comes from the return annotation. There is no `dtype=` keyword to + keep in step with the code - `-> float` is one real annotation, checked by your + type checker as well as by FastCS. + +## Writing as well as reading + +PyTango pairs a getter with a `@x.write` method (or `@x.setter` in the +`attribute` decorator form). FastCS mirrors `@property`: + +```python +class PowerSupply(Controller): + @attr(units="V", precision=3) + async def voltage(self) -> float: + """Output voltage.""" + return float(await self._conn.query("V?")) + + @voltage.setter + async def voltage(self, value: float) -> None: + await self._conn.send(f"V={value}") +``` + +A getter alone gives you a read-only `AttrR`; adding a setter makes the same +name an `AttrRW`. There is no write-only decorator - a write-only attribute is +rare enough to be written longhand as `AttrW(setter=...)`. + +The getter's docstring becomes the attribute's description, and keyword +arguments to `@attr` are the attribute's metadata - `units`, `precision`, +`limits`, `group`, `description`. They are checked against the datatype the +getter returns, so `precision` on a `-> str` getter is an error rather than a +field that is silently ignored. + +:::{note} +Type checkers special-case the builtin `property` but not decorators that +imitate it, so pyright reports the getter as *obscured by a declaration of the +same name*, and mypy as *already defined*. The two declarations are deliberate, +so silence it at the getter: + +```python +@attr(units="V") +async def voltage(self) -> float: # pyright: ignore[reportRedeclaration] + ... +``` + +Only read-write attributes need this. A read-only `@attr` declares its name +once and needs nothing. +::: + +## Deciding when a value is read + +PyTango polls an attribute on a period configured per device, outside the code. +In FastCS the schedule is part of the declaration, and is the same +`Polled`/`NotPolled` vocabulary the procedural form uses: + +```python +from fastcs.attributes import NotPolled, Polled, attr + + +class PowerSupply(Controller): + @attr(Polled(period=0.5), units="V") + async def voltage(self) -> float: + """Read every half second, because the device changes it.""" + return float(await self._conn.query("V?")) + + @attr + async def serial_number(self) -> str: + """Read once, when the controller connects.""" + return await self._conn.query("*IDN?") + + @attr(NotPolled()) + async def last_error(self) -> str: + """Never read on a schedule - only when something asks for it.""" + return await self._conn.query("ERR?") +``` + +A bare `@attr` means read once, at connect - the same default a bare +`getter=` has. See [](./update-attributes-from-device.md) for the whole picture, +including devices that push values at you rather than being polled. + +## Commands + +PyTango's `@command` and FastCS's `@command` line up directly, including typed +arguments and return values: + +```python +from fastcs.methods import command + + +class PowerSupply(Controller): + @command() + async def reset(self) -> None: + """Return the supply to its power-on state.""" + await self._conn.send("*RST") +``` + +See [](./typed-commands.md) for arguments and return values, and which +transports can serve them. + +## When not to use `@attr` + +`@attr` is the simple case: one attribute, one device call, known at the time +you write the class. It is sugar over the procedural form, and there are two +other spellings for when it stops fitting: + +- **The attribute needs more than a getter and a setter** - a shared connection + object, several attributes built in a loop, values that come from one + request - build them in `__init__` with `AttrR(getter=...)` / + `AttrRW(getter=..., setter=...)` directly. `@attr` degrades into exactly that + form, so nothing is lost by moving. +- **The device describes itself** - the attributes are discovered by asking the + device what it has, rather than written out. Declare what your code refers to + as type hints and let the controller fill them in at initialisation. See + [](../tutorials/dynamic-drivers.md). + +There is no free-function `attr()` factory: outside a class body, write the +constructor. diff --git a/src/fastcs/attributes/__init__.py b/src/fastcs/attributes/__init__.py index b9e08edc..80b84841 100644 --- a/src/fastcs/attributes/__init__.py +++ b/src/fastcs/attributes/__init__.py @@ -1,3 +1,8 @@ +from .attr_decorator import UnboundAttr as UnboundAttr +from .attr_decorator import UnboundAttrRW as UnboundAttrRW +from .attr_decorator import UnboundGetter as UnboundGetter +from .attr_decorator import UnboundSetter as UnboundSetter +from .attr_decorator import attr as attr from .attr_r import AttrR as AttrR from .attr_r import Getter as Getter from .attr_r import NotPolled as NotPolled diff --git a/src/fastcs/attributes/attr_decorator.py b/src/fastcs/attributes/attr_decorator.py new file mode 100644 index 00000000..34d42895 --- /dev/null +++ b/src/fastcs/attributes/attr_decorator.py @@ -0,0 +1,348 @@ +"""``@attr`` decorator sugar over the getter/setter constructors (ADR 0018). + +``@attr`` is the one-decorated-getter spelling a PyTango user expects, written +over the same machinery as the procedural ``AttrR(getter=...)`` / +``AttrRW(getter=..., setter=...)`` form rather than beside it. It is a +decorator only - there is no free-function ``attr()`` factory, and no +``@attr_r``/``@attr_rw``: an ``AttrR`` is a decorated getter, an ``AttrRW`` is +that plus a ``@x.setter``, and a write-only ``AttrW`` is rare enough to write +longhand. + +Binding follows ``@command``/``@scan``: the class body holds an `UnboundAttr` +describing the attribute, and each controller instance gets a fresh +``AttrR``/``AttrRW`` built from it at construction time. Nothing is deepcopied +from a class-scope prototype, so two instances of a controller never share an +attribute. +""" + +from __future__ import annotations + +from asyncio import iscoroutinefunction +from collections.abc import Awaitable, Callable +from inspect import Parameter, Signature, getdoc, signature +from types import MethodType +from typing import Any, Generic, Unpack, cast, overload + +from fastcs.attributes._infer_datatype import ( + _datatype_for_annotation, + _unwrap_update_annotation, +) +from fastcs.attributes.attr_r import AttrR, NotPolled, Polled, Schedule +from fastcs.attributes.attr_rw import AttrRW +from fastcs.attributes.update import Update +from fastcs.datatypes import DType_T, Meta +from fastcs.util import Controller_T + +UnboundGetter = Callable[[Controller_T], Awaitable[DType_T | Update[DType_T]]] +"""An ``@attr`` getter, taking the `Controller` it will be bound to as ``self``""" +UnboundSetter = Callable[ + [Controller_T, DType_T], Awaitable[None | DType_T | Update[DType_T]] +] +"""An ``@x.setter`` setter, taking the `Controller` it will be bound to as ``self``""" + + +def _type_name(datatype: Any) -> str: + """A datatype as it was most likely written, to name it in an error.""" + return getattr(datatype, "__name__", None) or repr(datatype) + + +def _summary(docstring: str | None) -> str | None: + """The first paragraph of a docstring, as a single line. + + A description is the one-line label a transport shows next to the value, so + a longer docstring carries only its summary into one. + """ + if not docstring: + return None + + return " ".join(docstring.split("\n\n", 1)[0].split()) or None + + +def _method_signature(fn: Callable, expected: int, kind: str) -> Signature: + """The signature of an ``@attr`` getter or setter, once it is known to be one. + + Args: + fn: The decorated function + expected: How many parameters it takes, including the ``self`` it is + bound to - one for a getter, two for a setter + kind: What the function is, to name it in errors + + Returns: + The signature, with its annotations resolved + + Raises: + TypeError: If the function is not an async method of the right arity + + """ + if not iscoroutinefunction(fn): + raise TypeError(f"@attr {kind} {fn.__qualname__} must be an async function") + + fn_signature = signature(fn, eval_str=True) + parameters = list(fn_signature.parameters.values()) + takes = "self" if expected == 1 else "self and the value to set" + if len(parameters) != expected or any( + parameter.kind in (Parameter.VAR_POSITIONAL, Parameter.VAR_KEYWORD) + for parameter in parameters + ): + raise TypeError( + f"@attr {kind} {fn.__qualname__} must be a method taking {takes}" + ) + + return fn_signature + + +class UnboundAttr(Generic[Controller_T, DType_T]): + """An ``@attr``-decorated getter, and the metadata that goes with it. + + An instance of this class lives in the `Controller` class body, in place of + the method it decorates. It is a declaration rather than an attribute: each + `Controller` instance binds it into an ``AttrR`` of its own during + construction, so the getter is bound to that instance and nothing is shared + between instances. + + It is a (non-data) descriptor only so that the attribute reads as the + ``AttrR`` it becomes - ``self.voltage.readback`` rather than the + declaration. Once the controller has bound it the attribute is in the + instance dictionary, which a non-data descriptor does not intercept, so + ``__get__`` runs only before binding. + """ + + def __init__( + self, + getter: UnboundGetter[Controller_T, DType_T], + schedule: Schedule[DType_T] | None = None, + meta: Meta | None = None, + setter: UnboundSetter[Controller_T, DType_T] | None = None, + ) -> None: + getter_signature = _method_signature(getter, expected=1, kind="getter") + + datatype = _datatype_for_annotation( + _unwrap_update_annotation(getter_signature.return_annotation) + ) + if datatype is None: + raise TypeError( + f"@attr getter {getter.__qualname__} must annotate the datatype " + "the attribute holds as its return type, for example `-> float`" + ) + + if isinstance(schedule, Polled | NotPolled) and schedule.getter is not None: + raise TypeError( + f"The schedule given to @attr on {getter.__qualname__} already " + "has a getter; pass a bare Polled(period=...) or NotPolled()" + ) + + self._getter = getter + self._setter = setter + self._schedule = schedule + self._datatype = datatype + self._meta: dict[str, Any] = dict(meta or {}) + self._name = getter.__name__ + + def __set_name__(self, owner: type, name: str) -> None: + self._name = name + + @overload + def __get__( + self, instance: None, owner: type | None = None, / + ) -> UnboundAttr[Controller_T, DType_T]: ... + + @overload + def __get__( + self, instance: object, owner: type | None = None, / + ) -> AttrR[DType_T]: ... + + def __get__(self, instance: Any, owner: type | None = None, /) -> Any: + if instance is None: + return self + + raise AttributeError( + f"Attribute '{self._name}' does not exist yet. An @attr declaration " + "becomes an attribute when the controller is constructed, so it " + "cannot be reached before Controller.__init__ has run." + ) + + @property + def datatype(self) -> Any: + """The datatype inferred from the getter's return annotation.""" + return self._datatype + + def has_setter(self) -> bool: + return self._setter is not None + + def setter( + self, fn: UnboundSetter[Controller_T, DType_T] + ) -> UnboundAttrRW[Controller_T, DType_T]: + """Declare the writer half, making this an ``AttrRW``. + + Mirrors ``@property``/``@x.setter``, so a read-write attribute is one + name with two decorated methods:: + + @voltage.setter + async def voltage(self, value: float) -> None: + await self._conn.send(f"V={value}") + + Args: + fn: The setter, taking ``self`` and the value to apply + + Returns: + A new `UnboundAttrRW` with the setter attached. This one is left + alone, so a subclass declaring a setter does not also give one to + the base class it inherited the getter from. + + Raises: + TypeError: If the setter is not an async method taking a value, or + annotates a value of a different datatype to the getter's + + """ + if self._setter is not None: + raise TypeError( + f"@attr getter {self._getter.__qualname__} already has a setter" + ) + + setter_signature = _method_signature(fn, expected=2, kind="setter") + + value = list(setter_signature.parameters.values())[1] + if value.annotation is not Signature.empty: + if _datatype_for_annotation(value.annotation) is not self._datatype: + raise TypeError( + f"@attr setter {fn.__qualname__} takes a " + f"{_type_name(value.annotation)}, but its getter returns a " + f"{_type_name(self._datatype)}" + ) + + return UnboundAttrRW( + self._getter, + schedule=self._schedule, + meta=cast(Meta, self._meta), + setter=fn, + ) + + def bind(self, controller: Controller_T) -> AttrR[DType_T]: + """Build the attribute this declares, for one `Controller` instance. + + Args: + controller: The controller whose methods the getter and setter are + + Returns: + An ``AttrR``, or an ``AttrRW`` if a setter was declared + + """ + getter = MethodType(self._getter, controller) + scheduled = getter if self._schedule is None else self._schedule(getter) + + meta = dict(self._meta) + if "description" not in meta: + description = _summary(getdoc(self._getter)) + if description is not None: + meta["description"] = description + + if self._setter is None: + attribute = AttrR(self._datatype, getter=scheduled, **meta) + else: + attribute = AttrRW( + self._datatype, + getter=scheduled, + setter=MethodType(self._setter, controller), + **meta, + ) + + return cast(AttrR[DType_T], attribute) + + def __repr__(self) -> str: + access_mode = "rw" if self._setter is not None else "r" + return ( + f"{type(self).__name__}({self._getter.__qualname__}, " + f"access_mode={access_mode!r}, datatype={_type_name(self._datatype)})" + ) + + +class UnboundAttrRW(UnboundAttr[Controller_T, DType_T]): + """An `UnboundAttr` that has been given a setter, so it binds an ``AttrRW``. + + A separate class only so that a declaration carrying a setter reads as the + ``AttrRW`` it becomes, and one without it as an ``AttrR``. + """ + + @overload # pyright: ignore[reportIncompatibleMethodOverride] + def __get__( + self, instance: None, owner: type | None = None, / + ) -> UnboundAttrRW[Controller_T, DType_T]: ... + + @overload + def __get__( + self, instance: object, owner: type | None = None, / + ) -> AttrRW[DType_T]: ... + + def __get__(self, instance: Any, owner: type | None = None, /) -> Any: + return super().__get__(instance, owner) + + def bind(self, controller: Controller_T) -> AttrRW[DType_T]: + return cast(AttrRW[DType_T], super().bind(controller)) + + +@overload +def attr( + getter: UnboundGetter[Controller_T, DType_T], / +) -> UnboundAttr[Controller_T, DType_T]: ... + + +@overload +def attr( + schedule: Schedule[Any] | None = None, /, **meta: Unpack[Meta] +) -> Callable[ + [UnboundGetter[Controller_T, DType_T]], UnboundAttr[Controller_T, DType_T] +]: ... + + +def attr(getter_or_schedule: Any = None, /, **meta: Any) -> Any: + """Declare an `Attribute` from the method that reads it. + + The datatype is the getter's return annotation and the getter's docstring + is the attribute's description, so the common "one attribute, one device + call" case is a single decorated method:: + + class PowerSupply(Controller): + @attr(Polled(period=0.5), units="V") + async def voltage(self) -> float: + \"\"\"Output voltage.\"\"\" + return float(await self._conn.query("V?")) + + @voltage.setter + async def voltage(self, value: float) -> None: + await self._conn.send(f"V={value}") + + The optional leading positional argument is a schedule - the same + `Polled`/`NotPolled` objects the procedural form wraps its getter in, so + the two spellings share one vocabulary: + + - ``@attr(units="V")`` is read once, when the controller connects, which is + what a bare ``getter=`` means and what a bare ``@attr`` means + - ``@attr(Polled(period=0.5))`` is read every 0.5 seconds, as + ``AttrR(getter=Polled(g, period=0.5))`` is + - ``@attr(NotPolled())`` is never read on a schedule, as + ``AttrR(getter=NotPolled(g))`` is + + Args: + getter_or_schedule: The getter, when used bare as ``@attr``; otherwise + a `Polled` or `NotPolled` schedule, or nothing + meta: Metadata for the attribute, checked against the datatype the + getter returns - ``precision`` on a ``str`` attribute raises + + Returns: + An `UnboundAttr`, which each `Controller` instance binds into an + attribute of its own + + """ + if getter_or_schedule is not None and not isinstance( + getter_or_schedule, Polled | NotPolled + ): + # Bare ``@attr``, so what we have is the getter itself. There is no way + # to pass metadata in that form, so there is none to carry over. + return UnboundAttr(getter_or_schedule) + + def wrapper( + getter: UnboundGetter[Controller_T, DType_T], + ) -> UnboundAttr[Controller_T, DType_T]: + return UnboundAttr(getter, schedule=getter_or_schedule, meta=cast(Meta, meta)) + + return wrapper diff --git a/src/fastcs/controllers/base_controller.py b/src/fastcs/controllers/base_controller.py index 05f207c2..ef3b7625 100755 --- a/src/fastcs/controllers/base_controller.py +++ b/src/fastcs/controllers/base_controller.py @@ -1,6 +1,7 @@ from __future__ import annotations from copy import deepcopy +from inspect import getattr_static from typing import ( TypeVar, _GenericAlias, # type: ignore @@ -9,7 +10,7 @@ get_type_hints, ) -from fastcs.attributes import Attribute, HintedAttribute +from fastcs.attributes import Attribute, HintedAttribute, UnboundAttr from fastcs.controllers.controller_api import ControllerAPI from fastcs.logging import logger from fastcs.methods import Command, Method, Scan, UnboundCommand, UnboundScan @@ -117,6 +118,9 @@ def _bind_attrs(self) -> None: controller class. For Methods, this requires creating a bound method from a class method and a controller instance, so that it can be called from any context with the controller instance passed as the ``self`` argument. + An ``@attr``-decorated getter is an `UnboundAttr` declaration rather than + an Attribute, and is bound the same way the Methods are - into a fresh + Attribute whose getter and setter are methods of this instance. """ class_dir = dict.fromkeys(self._walk_mro()) @@ -130,6 +134,13 @@ class method and a controller instance, so that it can be called from any if attr_name == "root_attribute": continue + # An ``UnboundAttr`` is a descriptor that refuses to be read before + # it is bound, so reach past it to the declaration itself. + declaration = getattr_static(self, attr_name, None) + if isinstance(declaration, UnboundAttr): + self.add_attribute(attr_name, declaration.bind(self)) + continue + attr = getattr(self, attr_name, None) if isinstance(attr, Attribute): setattr(self, attr_name, deepcopy(attr)) diff --git a/tests/test_attr_decorator.py b/tests/test_attr_decorator.py new file mode 100644 index 00000000..aecee93b --- /dev/null +++ b/tests/test_attr_decorator.py @@ -0,0 +1,404 @@ +import asyncio +from enum import Enum + +import numpy as np +import pytest + +from fastcs.attributes import ( + AttrR, + AttrRW, + NotPolled, + Polled, + UnboundAttr, + Update, + attr, +) +from fastcs.controllers import Controller +from fastcs.datatypes import Array1D, Limits, NumericLimits +from fastcs.util import ONCE + + +class State(Enum): + IDLE = "idle" + BUSY = "busy" + + +class PowerSupply(Controller): + """A controller declaring its attributes with ``@attr``.""" + + def __init__(self) -> None: + super().__init__() + + self.sent: list[float] = [] + self._voltage = 1.5 + + @attr(Polled(period=0.5), units="V", precision=3) + async def voltage(self) -> float: # pyright: ignore[reportRedeclaration] + """Output voltage. + + The rest of the docstring says more than a description should. + """ + return self._voltage + + @voltage.setter + async def voltage(self, value: float) -> None: + self.sent.append(value) + self._voltage = value + + @attr + async def serial(self) -> str: + """Serial number.""" + return "PSU-1" + + @attr(NotPolled(), group="Config") + async def retries(self) -> int: + return 3 + + +def test_getter_only_is_read_only(): + controller = PowerSupply() + + assert isinstance(controller.serial, AttrR) + assert not isinstance(controller.serial, AttrRW) + assert controller.serial.dtype is str + assert controller.serial.access_mode == "r" + + +def test_getter_and_setter_is_read_write(): + controller = PowerSupply() + + assert isinstance(controller.voltage, AttrRW) + assert controller.voltage.dtype is float + assert controller.voltage.access_mode == "rw" + + +def test_attributes_are_registered_with_the_controller(): + controller = PowerSupply() + + assert list(controller.attributes) == ["voltage", "serial", "retries"] + assert controller.attributes["voltage"] is controller.voltage + assert controller.voltage.name == "voltage" + + +def test_metadata_from_decorator(): + controller = PowerSupply() + + assert controller.voltage.meta == { + "units": "V", + "precision": 3, + "description": "Output voltage.", + } + assert controller.retries.meta == {"group": "Config"} + assert controller.retries.group == "Config" + + +def test_docstring_summary_becomes_the_description(): + controller = PowerSupply() + + # Only the first paragraph - a description is a one-line label. + assert controller.voltage.description == "Output voltage." + assert controller.serial.description == "Serial number." + assert controller.retries.description is None + + +def test_explicit_description_wins_over_the_docstring(): + class Device(Controller): + @attr(description="From the decorator") + async def label(self) -> str: + """From the docstring.""" + return "x" + + assert Device().label.description == "From the decorator" + + +def test_schedules(): + controller = PowerSupply() + + assert controller.voltage.poll_period == 0.5 + # A bare ``@attr`` means what a bare ``getter=`` means - read once, at connect. + assert controller.serial.poll_period is ONCE + assert controller.retries.poll_period is None + assert controller.retries.has_getter() + + +@pytest.mark.asyncio +async def test_bound_getter_reads_from_its_own_instance(): + one, two = PowerSupply(), PowerSupply() + two._voltage = 9.0 + + assert await one.voltage.poll() == 1.5 + assert await two.voltage.poll() == 9.0 + + +@pytest.mark.asyncio +async def test_bound_setter_writes_to_its_own_instance(): + one, two = PowerSupply(), PowerSupply() + + await one.voltage.set(2.5) + + assert one.sent == [2.5] + assert one._voltage == 2.5 + assert two.sent == [] + assert two._voltage == 1.5 + + +def test_each_instance_gets_a_fresh_attribute(): + one, two = PowerSupply(), PowerSupply() + + assert one.voltage is not two.voltage + assert one.serial is not two.serial + + +def test_class_body_holds_the_declaration(): + assert isinstance(PowerSupply.voltage, UnboundAttr) + assert PowerSupply.voltage.datatype is float + assert PowerSupply.voltage.has_setter() + assert not PowerSupply.serial.has_setter() + assert "PowerSupply.voltage" in repr(PowerSupply.voltage) + assert "access_mode='rw'" in repr(PowerSupply.voltage) + + +def test_datatype_inferred_from_the_return_annotation(): + class Device(Controller): + @attr + async def flag(self) -> bool: + return True + + @attr + async def state(self) -> State: + return State.IDLE + + @attr(shape=(4,)) + async def trace(self) -> Array1D[np.int32]: + return np.zeros(4, dtype=np.int32) + + controller = Device() + + assert controller.flag.dtype is bool + assert controller.state.dtype is State + assert controller.trace.dtype is np.ndarray + assert controller.trace.meta == {"array_dtype": np.int32, "shape": (4,)} + + +@pytest.mark.asyncio +async def test_update_return_annotation_is_unwrapped(): + class Device(Controller): + @attr + async def temperature(self) -> Update[float]: + return Update(readback=20.5, timestamp=1000.0) + + controller = Device() + + assert controller.temperature.dtype is float + assert await controller.temperature.poll() == 20.5 + assert controller.temperature.timestamp == 1000.0 + + +def test_metadata_is_validated_against_the_inferred_datatype(): + with pytest.raises(TypeError, match="'precision' is not valid metadata"): + + class Device(Controller): + @attr(precision=3) + async def label(self) -> str: + return "x" + + Device() + + +def test_limits_metadata(): + class Device(Controller): + @attr(limits=NumericLimits(control=Limits(0.0, 10.0))) + async def setpoint(self) -> float: + return 1.0 + + assert Device().setpoint.meta.get("limits") == NumericLimits( + control=Limits(0.0, 10.0) + ) + + +def test_matching_type_hint_is_satisfied_by_the_decorated_attribute(): + class Device(Controller): + label: AttrR[str] # pyright: ignore[reportRedeclaration] + + @attr + async def label(self) -> str: + return "x" + + controller = Device() + controller.post_initialise() + + assert isinstance(controller.label, AttrR) + + +def test_type_hint_of_the_wrong_access_mode_raises(): + with pytest.raises(RuntimeError, match="does not match defined access mode"): + + class Device(Controller): + label: AttrRW[str] # pyright: ignore[reportRedeclaration] + + @attr + async def label(self) -> str: + return "x" + + Device() + + +def test_name_clash_with_an_attribute_added_later_raises(): + class Device(Controller): + def __init__(self) -> None: + super().__init__() + + self.label = AttrR(str) # pyright: ignore[reportAttributeAccessIssue] + + @attr + async def label(self) -> str: + return "x" + + with pytest.raises(ValueError, match="Cannot add attribute") as exc_info: + Device() + + assert "has existing attribute label" in str(exc_info.value.__cause__) + + +def test_getter_must_be_async(): + with pytest.raises(TypeError, match="getter .* must be an async function"): + + @attr # pyright: ignore[reportArgumentType, reportCallIssue] + def voltage(self) -> float: + return 0.0 + + +def test_getter_must_take_only_self(): + with pytest.raises(TypeError, match="getter .* must be a method taking self"): + + @attr() # pyright: ignore[reportArgumentType] + async def voltage(self, index: int) -> float: + return 0.0 + + +def test_getter_must_annotate_its_return_type(): + with pytest.raises(TypeError, match="must annotate the datatype"): + + @attr() + async def voltage(self): + return 0.0 + + +def test_getter_must_return_a_supported_datatype(): + with pytest.raises(TypeError, match="must annotate the datatype"): + + @attr() # pyright: ignore[reportArgumentType] + async def voltage(self) -> list[int]: + return [] + + +def test_setter_must_be_async(): + @attr + async def voltage(self) -> float: # pyright: ignore[reportRedeclaration] + return 0.0 + + with pytest.raises(TypeError, match="setter .* must be an async function"): + + @voltage.setter # pyright: ignore[reportArgumentType] + def voltage(self, value: float) -> None: + pass + + +def test_setter_must_take_a_value(): + @attr + async def voltage(self) -> float: # pyright: ignore[reportRedeclaration] + return 0.0 + + with pytest.raises( + TypeError, match="setter .* must be a method taking self and the value to set" + ): + + @voltage.setter # pyright: ignore[reportArgumentType] + async def voltage(self) -> None: + pass + + +def test_setter_value_must_match_the_getter_datatype(): + @attr + async def voltage(self) -> float: # pyright: ignore[reportRedeclaration] + return 0.0 + + with pytest.raises(TypeError, match="takes a str, but its getter returns a float"): + + @voltage.setter # pyright: ignore[reportArgumentType] + async def voltage(self, value: str) -> None: + pass + + +def test_setter_value_annotation_is_optional(): + @attr + async def voltage(self) -> float: # pyright: ignore[reportRedeclaration] + return 0.0 + + voltage = voltage.setter(_untyped_setter) + + assert voltage.has_setter() + + +async def _untyped_setter(self, value) -> None: + pass + + +def test_only_one_setter(): + @attr + async def voltage(self) -> float: # pyright: ignore[reportRedeclaration] + return 0.0 + + @voltage.setter + async def voltage(self, value: float) -> None: + pass + + with pytest.raises(TypeError, match="already has a setter"): + + @voltage.setter + async def voltage(self, value: float) -> None: + pass + + +def test_setter_does_not_leak_onto_the_class_it_was_inherited_from(): + class Base(Controller): + @attr + async def voltage(self) -> float: + return 0.0 + + class Child(Base): + @Base.voltage.setter # pyright: ignore[reportArgumentType] + async def voltage(self, value: float) -> None: + pass + + assert not Base.voltage.has_setter() + assert Child.voltage.has_setter() + assert not isinstance(Base().voltage, AttrRW) + assert isinstance(Child().voltage, AttrRW) + + +def test_schedule_must_not_already_have_a_getter(): + async def read() -> float: + return 0.0 + + with pytest.raises(TypeError, match="already has a getter"): + + @attr(Polled(read, period=0.1)) + async def voltage(self) -> float: + return 0.0 + + +@pytest.mark.asyncio +async def test_polled_attributes_are_scheduled(): + controller = PowerSupply() + _, periodic, initial = controller.create_api_and_tasks() + + # ``serial`` is read once at connect; ``voltage`` is polled at 0.5s; + # ``retries`` is never read on a schedule. + assert len(initial) == 1 + assert len(periodic) == 1 + + await asyncio.gather(*[coro() for coro in initial]) + + assert controller.serial.readback == "PSU-1" + assert controller.retries.readback == 0 From 8064b70fb4835332471b0b65e16e5a60ad855c3a Mon Sep 17 00:00:00 2001 From: Shihab Suliman Date: Fri, 4 Sep 2026 10:45:57 +0000 Subject: [PATCH 2/4] chore: amend helper function to not do the job of the caller in raising specific exceptions; instead, reraise. --- src/fastcs/attributes/attr_decorator.py | 60 ++++++++++++++----------- 1 file changed, 34 insertions(+), 26 deletions(-) diff --git a/src/fastcs/attributes/attr_decorator.py b/src/fastcs/attributes/attr_decorator.py index 34d42895..905e7750 100644 --- a/src/fastcs/attributes/attr_decorator.py +++ b/src/fastcs/attributes/attr_decorator.py @@ -58,37 +58,23 @@ def _summary(docstring: str | None) -> str | None: return " ".join(docstring.split("\n\n", 1)[0].split()) or None -def _method_signature(fn: Callable, expected: int, kind: str) -> Signature: - """The signature of an ``@attr`` getter or setter, once it is known to be one. +def _method_signature(fn: Callable) -> Signature: + """Resolve the signature of an async ``@attr`` getter or setter. Args: fn: The decorated function - expected: How many parameters it takes, including the ``self`` it is - bound to - one for a getter, two for a setter - kind: What the function is, to name it in errors Returns: The signature, with its annotations resolved Raises: - TypeError: If the function is not an async method of the right arity + TypeError: If the function is not an async method """ if not iscoroutinefunction(fn): - raise TypeError(f"@attr {kind} {fn.__qualname__} must be an async function") - - fn_signature = signature(fn, eval_str=True) - parameters = list(fn_signature.parameters.values()) - takes = "self" if expected == 1 else "self and the value to set" - if len(parameters) != expected or any( - parameter.kind in (Parameter.VAR_POSITIONAL, Parameter.VAR_KEYWORD) - for parameter in parameters - ): - raise TypeError( - f"@attr {kind} {fn.__qualname__} must be a method taking {takes}" - ) + raise TypeError("must be an async function") - return fn_signature + return signature(fn, eval_str=True) class UnboundAttr(Generic[Controller_T, DType_T]): @@ -114,12 +100,25 @@ def __init__( meta: Meta | None = None, setter: UnboundSetter[Controller_T, DType_T] | None = None, ) -> None: - getter_signature = _method_signature(getter, expected=1, kind="getter") - - datatype = _datatype_for_annotation( - _unwrap_update_annotation(getter_signature.return_annotation) - ) + try: + getter_signature = _method_signature(getter) + getter_parameters = list(getter_signature.parameters.values()) + if len(getter_parameters) != 1 or any( + parameter.kind in (Parameter.VAR_POSITIONAL, Parameter.VAR_KEYWORD) + for parameter in getter_parameters + ): + raise TypeError("must be a method taking self") + except TypeError as error: + raise TypeError(f"@attr getter {getter.__qualname__} {error}") from error + + annotation = _unwrap_update_annotation(getter_signature.return_annotation) + datatype = _datatype_for_annotation(annotation) if datatype is None: + if annotation is not Signature.empty: + raise TypeError( + f"@attr getter {getter.__qualname__} must annotate a supported " + f"datatype, got {_type_name(annotation)}" + ) raise TypeError( f"@attr getter {getter.__qualname__} must annotate the datatype " "the attribute holds as its return type, for example `-> float`" @@ -199,7 +198,16 @@ async def voltage(self, value: float) -> None: f"@attr getter {self._getter.__qualname__} already has a setter" ) - setter_signature = _method_signature(fn, expected=2, kind="setter") + try: + setter_signature = _method_signature(fn) + setter_parameters = list(setter_signature.parameters.values()) + if len(setter_parameters) != 2 or any( + parameter.kind in (Parameter.VAR_POSITIONAL, Parameter.VAR_KEYWORD) + for parameter in setter_parameters + ): + raise TypeError("must be a method taking self and the value to set") + except TypeError as error: + raise TypeError(f"@attr setter {fn.__qualname__} {error}") from error value = list(setter_signature.parameters.values())[1] if value.annotation is not Signature.empty: @@ -263,7 +271,7 @@ class UnboundAttrRW(UnboundAttr[Controller_T, DType_T]): ``AttrRW`` it becomes, and one without it as an ``AttrR``. """ - @overload # pyright: ignore[reportIncompatibleMethodOverride] + @overload def __get__( self, instance: None, owner: type | None = None, / ) -> UnboundAttrRW[Controller_T, DType_T]: ... From cdcb35f64ce43278b63cfb948c38be4f8164cecf Mon Sep 17 00:00:00 2001 From: Shihab Suliman Date: Fri, 4 Sep 2026 10:46:31 +0000 Subject: [PATCH 3/4] tests: fix code smells in tests such as useless lines and more than one line in pytest.raises --- tests/test_attr_decorator.py | 37 ++++++++++++++++-------------------- 1 file changed, 16 insertions(+), 21 deletions(-) diff --git a/tests/test_attr_decorator.py b/tests/test_attr_decorator.py index aecee93b..711cca1c 100644 --- a/tests/test_attr_decorator.py +++ b/tests/test_attr_decorator.py @@ -195,13 +195,12 @@ async def temperature(self) -> Update[float]: def test_metadata_is_validated_against_the_inferred_datatype(): - with pytest.raises(TypeError, match="'precision' is not valid metadata"): - - class Device(Controller): - @attr(precision=3) - async def label(self) -> str: - return "x" + class Device(Controller): + @attr(precision=3) + async def label(self) -> str: + return "x" + with pytest.raises(TypeError, match="'precision' is not valid metadata"): Device() @@ -225,21 +224,19 @@ async def label(self) -> str: return "x" controller = Device() - controller.post_initialise() assert isinstance(controller.label, AttrR) def test_type_hint_of_the_wrong_access_mode_raises(): - with pytest.raises(RuntimeError, match="does not match defined access mode"): - - class Device(Controller): - label: AttrRW[str] # pyright: ignore[reportRedeclaration] + class Device(Controller): + label: AttrRW[str] # pyright: ignore[reportRedeclaration] - @attr - async def label(self) -> str: - return "x" + @attr + async def label(self) -> str: + return "x" + with pytest.raises(RuntimeError, match="does not match defined access mode"): Device() @@ -263,7 +260,7 @@ async def label(self) -> str: def test_getter_must_be_async(): with pytest.raises(TypeError, match="getter .* must be an async function"): - @attr # pyright: ignore[reportArgumentType, reportCallIssue] + @attr() # pyright: ignore[reportArgumentType] def voltage(self) -> float: return 0.0 @@ -285,7 +282,7 @@ async def voltage(self): def test_getter_must_return_a_supported_datatype(): - with pytest.raises(TypeError, match="must annotate the datatype"): + with pytest.raises(TypeError, match="must annotate a supported datatype"): @attr() # pyright: ignore[reportArgumentType] async def voltage(self) -> list[int]: @@ -335,15 +332,13 @@ def test_setter_value_annotation_is_optional(): async def voltage(self) -> float: # pyright: ignore[reportRedeclaration] return 0.0 - voltage = voltage.setter(_untyped_setter) + @voltage.setter + async def voltage(self, value) -> None: + pass assert voltage.has_setter() -async def _untyped_setter(self, value) -> None: - pass - - def test_only_one_setter(): @attr async def voltage(self) -> float: # pyright: ignore[reportRedeclaration] From a5b7bf71840aa95e2d1e96d66c850ccdc86d6a19 Mon Sep 17 00:00:00 2001 From: Shihab Suliman Date: Fri, 4 Sep 2026 10:46:56 +0000 Subject: [PATCH 4/4] chore: add claude.md file with issues found in this branch --- claude.md | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 117 insertions(+) create mode 100644 claude.md diff --git a/claude.md b/claude.md new file mode 100644 index 00000000..65c11785 --- /dev/null +++ b/claude.md @@ -0,0 +1,117 @@ +# CLAUDE.md + +Conventions for this repository. Follow these when writing or reviewing code and tests. + +## Helper functions and exceptions + +- A helper function must **not** be passed a parameter (e.g. `kind`) whose only purpose + is to be interpolated into the message of an exception it raises. That parameter + exists purely to serve one or more call-sites' error-reporting needs, which means the + helper is doing the call-site's job for it. +- If different call-sites need different exceptions raised (different types and/or + different messages), do not thread a parameter into the helper to cover every case. + Instead, let the helper raise its own plain/generic exception with no caller-supplied + wording, and have each call-site `catch` it and `raise ... from ...` with the + type/message it actually needs. + + ```python + # Bad: helper takes `kind` purely to phrase its own exception message + def _check_positive(value: float, kind: str) -> None: + if value <= 0: + raise ValueError(f"{kind} must be positive, got {value}") + + _check_positive(period, kind="period") + + # Good: helper raises a plain exception; call-site adds whatever context it needs + def _check_positive(value: float) -> None: + if value <= 0: + raise ValueError(f"must be positive, got {value}") + + try: + _check_positive(period) + except ValueError as e: + raise ConfigError(f"invalid period in trigger config: {e}") from e + ``` + +- A helper function must **not** be passed a parameter like `expected` or `skip` that + tells it about the *arity or shape of the call-site* (e.g. "how many items did you + expect", "should this check be skipped"). Parameters like these are a sign that the + check itself belongs in the caller, not the helper. Move the check up: + + ```python + # Bad: helper is making a decision that belongs to the caller + def _check_length(items, expected=None): + if expected is not None and len(items) != expected: + raise ValueError(...) + + # Good: caller owns the decision, helper just does the one thing it's for + if len(items) != expected: + raise ValueError(...) + _check_length(items) + ``` + + Rule of thumb: a helper's parameters should describe *what it's being asked to + validate/produce*, never *whether/how the caller wants it validated*. + +## Tests: `pytest.raises` + +- The `with pytest.raises(...):` block should contain the **minimal code that raises + the exception** — ideally a single line, and ideally just the call under test. +- Any setup needed to *put the system in a state* where that call will raise must + happen **outside** and **before** the `pytest.raises` block, not inside it. + + ```python + # Bad: setup is inside the raises block + with pytest.raises(ValueError): + controller = Device() + controller.configure(bad_value) + + # Good: setup happens first, only the failing call is inside the block + controller = Device() + with pytest.raises(ValueError): + controller.configure(bad_value) + ``` + + This keeps the assertion precise: if setup itself started raising unexpectedly, the + test should fail with an ordinary traceback, not be masked as a (possibly + coincidental) pass inside `pytest.raises`. + +## Tests: no irrelevant lines + +- Every line in a test should be there because it affects the test's outcome, given + what the test's name says it's checking. If removing a line wouldn't change whether + the test passes or fails, it doesn't belong. +- Before adding or keeping a line in a test, check it against the test name: does this + line change the behavior being verified? If not, delete it. + + ```python + # Bad: post_initialise() doesn't affect this test's assertion + def test_matching_type_hint_is_satisfied_by_the_decorated_attribute(): + class Device(Controller): + label: AttrR[str] # pyright: ignore[reportRedeclaration] + + @attr + async def label(self) -> str: + return "x" + + controller = Device() + controller.post_initialise() # irrelevant — remove + + assert isinstance(controller.label, AttrR) + + # Good + def test_matching_type_hint_is_satisfied_by_the_decorated_attribute(): + class Device(Controller): + label: AttrR[str] # pyright: ignore[reportRedeclaration] + + @attr + async def label(self) -> str: + return "x" + + controller = Device() + + assert isinstance(controller.label, AttrR) + ``` + + This keeps tests readable as documentation: every line is evidence for the claim in + the test's name, not incidental noise carried over from copy-pasting another test.