From ca8748f5ae8b93fd4ff988a44668f1621a34efc9 Mon Sep 17 00:00:00 2001 From: Tom Cobb Date: Thu, 28 Nov 2024 15:54:13 +0000 Subject: [PATCH] Fix review comments --- src/ophyd_async/core/_device.py | 32 ++++++++++++++++++++++------ src/ophyd_async/epics/demo/_mover.py | 4 ++-- src/ophyd_async/epics/motor.py | 4 ++-- tests/core/test_device.py | 15 +++++++------ 4 files changed, 39 insertions(+), 16 deletions(-) diff --git a/src/ophyd_async/core/_device.py b/src/ophyd_async/core/_device.py index a829964682..0439052db9 100644 --- a/src/ophyd_async/core/_device.py +++ b/src/ophyd_async/core/_device.py @@ -71,13 +71,16 @@ class Device(HasName, Connectable): _connect_task: asyncio.Task | None = None # The mock if we have connected in mock mode _mock: LazyMock | None = None + # The separator to use when making child names + _child_name_separator: str = "-" def __init__( self, name: str = "", connector: DeviceConnector | None = None ) -> None: self._connector = connector or DeviceConnector() self._connector.create_children_from_annotations(self) - self.set_name(name) + if name: + self.set_name(name) @property def name(self) -> str: @@ -97,21 +100,30 @@ def log(self) -> LoggerAdapter: getLogger("ophyd_async.devices"), {"ophyd_async_device_name": self.name} ) - def set_name(self, name: str, separator: str = "-"): + def set_name(self, name: str, *, child_name_separator: str | None = None) -> None: """Set ``self.name=name`` and each ``self.child.name=name+"-child"``. Parameters ---------- name: New name to set + child_name_separator: + Use this as a separator instead of "-". Use "_" instead to make the same + names as the equivalent ophyd sync device. """ self._name = name + if child_name_separator: + self._child_name_separator = child_name_separator # Ensure logger is recreated after a name change if "log" in self.__dict__: del self.log - for child_name, child in self.children(): - child_name = f"{self.name}{separator}{child_name}" if self.name else "" - child.set_name(child_name, separator) + for attr_name, child in self.children(): + child_name = ( + f"{self.name}{self._child_name_separator}{attr_name}" + if self.name + else "" + ) + child.set_name(child_name, child_name_separator=self._child_name_separator) def __setattr__(self, name: str, value: Any) -> None: # Bear in mind that this function is called *a lot*, so @@ -147,6 +159,10 @@ async def connect( timeout: Time to wait before failing with a TimeoutError. """ + assert hasattr(self, "_connector"), ( + f"{self}: doesn't have attribute `_connector`," + " did you call `super().__init__` in your `__init__` method?" + ) if mock: # Always connect in mock mode serially if isinstance(mock, LazyMock): @@ -247,6 +263,8 @@ class DeviceCollector: set_name: If True, call ``device.set_name(variable_name)`` on all collected Devices + child_name_separator: + Use this as a separator if we call ``set_name``. connect: If True, call ``device.connect(mock)`` in parallel on all collected Devices @@ -271,11 +289,13 @@ class DeviceCollector: def __init__( self, set_name=True, + child_name_separator: str = "-", connect=True, mock=False, timeout: float = 10.0, ): self._set_name = set_name + self._child_name_separator = child_name_separator self._connect = connect self._mock = mock self._timeout = timeout @@ -311,7 +331,7 @@ async def _on_exit(self) -> None: for name, obj in self._objects_on_exit.items(): if name not in self._names_on_enter and isinstance(obj, Device): if self._set_name and not obj.name: - obj.set_name(name) + obj.set_name(name, child_name_separator=self._child_name_separator) if self._connect: connect_coroutines[name] = obj.connect( self._mock, timeout=self._timeout diff --git a/src/ophyd_async/epics/demo/_mover.py b/src/ophyd_async/epics/demo/_mover.py index fa04a97f37..88bd3fd655 100644 --- a/src/ophyd_async/epics/demo/_mover.py +++ b/src/ophyd_async/epics/demo/_mover.py @@ -37,8 +37,8 @@ def __init__(self, prefix: str, name="") -> None: super().__init__(name=name) - def set_name(self, name: str, separator: str = "-"): - super().set_name(name, separator) + def set_name(self, name: str, *, child_name_separator: str | None = None) -> None: + super().set_name(name, child_name_separator=child_name_separator) # Readback should be named the same as its parent in read() self.readback.set_name(name) diff --git a/src/ophyd_async/epics/motor.py b/src/ophyd_async/epics/motor.py index d3732fbc4e..482adfd97c 100644 --- a/src/ophyd_async/epics/motor.py +++ b/src/ophyd_async/epics/motor.py @@ -91,8 +91,8 @@ def __init__(self, prefix: str, name="") -> None: super().__init__(name=name) - def set_name(self, name: str, separator: str = "-"): - super().set_name(name, separator) + def set_name(self, name: str, *, child_name_separator: str | None = None) -> None: + super().set_name(name, child_name_separator=child_name_separator) # Readback should be named the same as its parent in read() self.user_readback.set_name(name) diff --git a/tests/core/test_device.py b/tests/core/test_device.py index 7143c9f55c..9850da8eee 100644 --- a/tests/core/test_device.py +++ b/tests/core/test_device.py @@ -115,12 +115,15 @@ async def test_children_of_device_have_set_names_and_get_connected( async def test_children_of_device_with_different_separator( parent: DummyDeviceGroup, ): - parent.set_name("parent", separator="_") - assert parent.name == "parent" - assert parent.child1.name == "parent_child1" - assert parent._child2.name == "parent__child2" - assert parent.dict_with_children.name == "parent_dict_with_children" - assert parent.dict_with_children[123].name == "parent_dict_with_children_123" + for separator in ("_", None): + # The second time round, check that it doesn't change name if + # we pass None, as this is what PviConnector does + parent.set_name("parent", child_name_separator=separator) + assert parent.name == "parent" + assert parent.child1.name == "parent_child1" + assert parent._child2.name == "parent__child2" + assert parent.dict_with_children.name == "parent_dict_with_children" + assert parent.dict_with_children[123].name == "parent_dict_with_children_123" async def test_device_with_device_collector():