Repository navigation
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
New ABC inheritance introduces unsuppressed Ruff B024/B027 violations in movable and flyable logic.
2 open issues
What changed in this PR
This PR enforces abstract-method implementation across backend and device-logic base classes, making incomplete subclasses fail at construction.
Changes:
- Add
ABCinheritance to nine base classes. - Adapt Tango commands and unavailable EPICS transports for abstract enforcement.
- Update contributor guidance and add Tango command tests.
| File | Description |
|---|---|
| tests/system_tests/tango/core/test_tango_transport.py | Test unsupported command reads and subscription state. |
| src/ophyd_async/tango/core/_tango_transport.py | Enforce abstraction and implement missing command methods. |
| src/ophyd_async/epics/core/_signal.py | Preserve unavailable-transport errors through __new__. |
| src/ophyd_async/core/_soft_signal_backend.py | Make converters inherit ABC. |
| src/ophyd_async/core/_signal_backend.py | Enforce abstract signal backend methods. |
| src/ophyd_async/core/_settings.py | Enforce abstract settings provider methods. |
| src/ophyd_async/core/_movable.py | Add ABC to movable logic. |
| src/ophyd_async/core/_flyable.py | Enforce abstract flyable hooks. |
| src/ophyd_async/core/_data_providers.py | Enforce abstract data provider methods. |
| src/ophyd_async/core/_command.py | Enforce abstract command backend methods. |
| CLAUDE.md | Revise abstract-class guidance. |
Review effort: Balanced
There was a problem hiding this comment.
I think this sweep covers everything, except for StandardMovable which I see still does not raise if movable_logic is not implemented. I ran this test which fails:
def test_movable_without_movable_logic_raises():
class NoLogicMovable(StandardMovable[float]):
pass
with pytest.raises(TypeError, match="movable_logic"):
NoLogicMovable() # pyright: ignore[reportAbstractUsage]Looks like this is because movable_logic is a cached_property, so ABCMeta only finds the cached property object. property works though, but then we'd rebuild the object each call, unless a subclass ignores the pyright flag....requesting changes so we can discuss this more.
|
system tests failing because of an ubuntu base image update, investigating... |
|
shihab-dls
left a comment
shihab-dls
left a comment
There was a problem hiding this comment.
Looks good! 2 comments, both minor, so approving.
| async def connect(self) -> None: # noqa: B027 | ||
| """Perform actions after proxy is connected. |
There was a problem hiding this comment.
I'm assuming this is ignored instead of made an abstractmethod because connect() is an optional hook, right?
| @@ -152,6 +150,7 @@ def movable_logic(self) -> MovableLogic: | |||
| `readback` signals directly. Subclasses must implement this as a | |||
| `@cached_property` that returns a `MovableLogic` instance. | |||
| """ | |||
| raise NotImplementedError | |||
There was a problem hiding this comment.
Can't we just use ...? As ABCMeta already catches inavlid construction now
A plain class with @AbstractMethod is enforced neither by pyright nor at runtime: a subclass that misses a method silently runs the stub and returns None. Give every such base ABCMeta by inheriting ABC, and record the rule in CLAUDE.md. This exposed tango's CommandProxy, which implemented none of TangoProxy's get, get_reading and has_subscription: the first two now raise NotImplementedError and has_subscription returns False. Assisted-by: Claude Sonnet 5.5
TransportNotAvailable now inherits an ABC through EpicsSignalBackend, so
instantiating it fails the abstract-method check in object.__new__ with a
TypeError before __init__ can raise the intended NotImplementedError
("did you pip install ophyd_async[ca]?"). Raise it from __new__ instead.
Assisted-by: Claude Sonnet 5.5
Covers the get, get_reading and has_subscription methods added to CommandProxy so that it implements the now-ABC TangoProxy. Assisted-by: Claude Sonnet 5.5
Addresses shihab-dls's review of bluesky#1461: ABCMeta does not see an abstract cached_property, so StandardMovable without movable_logic did not raise. Adds abstract_cached_property and uses it for movable_logic. Also drops ABC from MovableLogic, which has no abstract methods. Assisted-by: Claude Opus 5
A class with
@abstractmethodonly has its abstract methods enforced when its metaclass isABCMeta.Genericand@dataclassdon't provide it, so on a plain base class neither pyright (reportAbstractUsage) nor the runtime notices a subclass that misses a method: the stub runs and returnsNone. The previousCLAUDE.mdguidance ("NoABCbase needed ... pyright enforces implementation statically") was wrong.This PR makes every such base in
src/inheritABC:CommandBackend,ReadableDataProvider,StreamableDataProvider,FlyableLogic,MovableLogic,SettingsProvider,SignalBackend,SoftConverterandTangoProxy.Protocols and classes that already usedABC(e.g.DetectorAcquireLogic) are unchanged.Doing so exposed a real bug: tango's
CommandProxyimplemented none ofTangoProxy'sget,get_readingandhas_subscription.getandget_readingnow raiseNotImplementedError, like its existing subscribe methods, andhas_subscriptionreturnsFalse.TangoProxy.connectis an intentional empty hook, so it gets# noqa: B027.The
CLAUDE.mdcode convention is rewritten: every class with an@abstractmethodhasABCMetaas its metaclass, viaABCunless a base already brings it (Protocol, pydanticBaseModel).Downstream subclasses that are missing an abstract method now fail with
TypeErrorat construction, instead of silently returningNonelater.AI Disclaimer
The code and this description were generated with AI and reviewed/tweaked by the author.