chore: address review comments

Author: shihab-dls

src/fastcs/attributes.py

@@ -109,7 +109,7 @@ def set_path(self, path: list[str]):
 
     def __repr__(self):
         name = self.__class__.__name__
-        path = ".".join([str(node) for node in self._path] + [self._name]) or None
+        path = ".".join(self._path + [self._name]) or None
         datatype = self._datatype.__class__.__name__
 
         return f"{name}(path={path}, datatype={datatype}, io_ref={self._io_ref})"

src/fastcs/controller.py

@@ -158,18 +158,18 @@ def add_sub_controller(
 
         sub_controller.set_path(self.path + [name])
         self.__sub_controller_tree[name] = sub_controller
-        super().__setattr__(str(name), sub_controller)
+        super().__setattr__(name, sub_controller)
 
         if isinstance(sub_controller.root_attribute, Attribute):
-            self.attributes[str(name)] = sub_controller.root_attribute
+            self.attributes[name] = sub_controller.root_attribute
 
     @property
     def sub_controllers(self) -> dict[str, BaseController]:
         return self.__sub_controller_tree
 
     def __repr__(self):
         name = self.__class__.__name__
-        path = ".".join([str(p) for p in self.path]) or None
+        path = ".".join(self.path) or None
         sub_controllers = list(self.sub_controllers.keys()) or None
 
         return f"{name}(path={path}, sub_controllers={sub_controllers})"
@@ -219,11 +219,8 @@ async def disconnect(self) -> None:
 
 
 class ControllerVector(MutableMapping[int, Controller], BaseController):
-    """A collection of SubControllers, with an arbitrary integer index.
-    An instance of this class can be registered with a parent ``Controller`` to include
-    it's children as part of a larger controller. Each child of the vector will keep
-    a string name of the vector.
-    """
+    """A controller with a collection of identical sub controllers distinguished
+    by a numeric value"""
 
     root_attribute: Attribute | None = None
 
@@ -239,14 +236,22 @@ def __init__(
         for index, child in children.items():
             super().add_sub_controller(str(index), child)
 
-    def add_sub_controller(self, *args, **kwargs):
+    def add_sub_controller(
+        self, name: str, sub_controller: Controller | ControllerVector
+    ):
         raise NotImplementedError(
             "Cannot add named sub controller to ControllerVector. "
-            "Use __setitem__ instead, for indexed sub controllers"
+            "Use __setitem__ instead, for indexed sub controllers. "
+            "E.g., vector[1] = Controller()"
         )
 
     def __getitem__(self, key: int) -> Controller:
-        return self._children[key]
+        try:
+            return self._children[key]
+        except KeyError as exception:
+            raise KeyError(
+                f"ControllerVector does not have Controller with key {key}"
+            ) from exception
 
     def __setitem__(self, key: int, value: Controller) -> None:
         if not isinstance(key, int):
@@ -266,9 +271,5 @@ def __iter__(self) -> Iterator[int]:
     def __len__(self) -> int:
         return len(self._children)
 
-    def children(self) -> Iterator[tuple[str, Controller]]:
-        for key, child in self._children.items():
-            yield str(key), child
-
-    def __hash__(self):
-        return hash(id(self))
+    def children(self) -> Iterator[tuple[int, Controller]]:
+        yield from self._children.items()

src/fastcs/controller_api.py

@@ -41,7 +41,7 @@ def __repr__(self):
         return (
             f"ControllerAPI("
             f"path={self.path}, "
-            f"sub_apis=[{', '.join(str(sub_api) for sub_api in self.sub_apis.keys())}]"
+            f"sub_apis=[{', '.join(sub_api for sub_api in self.sub_apis.keys())}]"
             f")"
         )
 

src/fastcs/transport/epics/ca/ioc.py

@@ -116,7 +116,7 @@ def _add_sub_controller_pvi_info(pv_prefix: str, parent: ControllerAPI):
         child_name = (
             f"__{child.path[-1]}"  # Sub-Controller of ControllerVector
             if child.path[-1].isdigit()
-            else str(child.path[-1])
+            else child.path[-1]
         )
 
         _add_pvi_info(child_pvi, parent_pvi, child_name.lower())

src/fastcs/transport/epics/gui.py

@@ -53,7 +53,7 @@ def __init__(self, controller_api: ControllerAPI, pv_prefix: str) -> None:
 
     def _get_pv(self, attr_path: list[str], name: str):
         attr_prefix = ":".join(
-            [self._pv_prefix] + [snake_to_pascal(str(node)) for node in attr_path]
+            [self._pv_prefix] + [snake_to_pascal(node) for node in attr_path]
         )
         return f"{attr_prefix}:{snake_to_pascal(name)}"
 

src/fastcs/transport/epics/pva/pvi_tree.py

@@ -42,7 +42,6 @@ class PviDevice(dict[str, "PviDevice"]):
     """For creating a pvi structure in pva."""
 
     pv_prefix: str
-    controller_api: ControllerAPI | None
     description: str | None
     device_signal_info: _PviSignalInfo | None
 

src/fastcs/transport/epics/util.py

@@ -3,6 +3,4 @@
 
 
 def controller_pv_prefix(prefix: str, controller_api: ControllerAPI) -> str:
-    return ":".join(
-        [prefix] + [snake_to_pascal(str(node)) for node in controller_api.path]
-    )
+    return ":".join([prefix] + [snake_to_pascal(node) for node in controller_api.path])

src/fastcs/transport/graphql/graphql.py

@@ -86,7 +86,7 @@ def _process_commands(self, controller_api: ControllerAPI):
     def _process_sub_apis(self, root_controller_api: ControllerAPI):
         """Recursively add fields from the queries and mutations of sub apis"""
         for controller_api in root_controller_api.sub_apis.values():
-            name = "".join([str(node) for node in controller_api.path])
+            name = "".join(controller_api.path)
             child_tree = GraphQLAPI(controller_api)
             if child_tree.queries:
                 self.queries.append(

src/fastcs/transport/rest/rest.py

@@ -101,7 +101,7 @@ async def attr_get() -> Any:  # Must be any as response_model is set
 
 def _add_attribute_api_routes(app: FastAPI, root_controller_api: ControllerAPI) -> None:
     for controller_api in root_controller_api.walk_api():
-        path = [str(node) for node in controller_api.path]
+        path = controller_api.path
 
         for attr_name, attribute in controller_api.attributes.items():
             attr_name = attr_name.replace("_", "-")
@@ -151,7 +151,7 @@ async def command() -> None:
 
 def _add_command_api_routes(app: FastAPI, root_controller_api: ControllerAPI) -> None:
     for controller_api in root_controller_api.walk_api():
-        path = [str(node) for node in controller_api.path]
+        path = controller_api.path
 
         for name, method in root_controller_api.command_methods.items():
             cmd_name = name.replace("_", "-")

src/fastcs/transport/tango/dsr.py

@@ -61,7 +61,7 @@ def _collect_dev_attributes(
 ) -> dict[str, Any]:
     collection: dict[str, Any] = {}
     for controller_api in root_controller_api.walk_api():
-        path = [str(node) for node in controller_api.path]
+        path = controller_api.path
 
         for attr_name, attribute in controller_api.attributes.items():
             attr_name = attr_name.title().replace("_", "")
@@ -109,8 +109,7 @@ def _wrap_command_f(
 ) -> Callable[..., Awaitable[None]]:
     async def _dynamic_f(tango_device: Device) -> None:
         tango_device.info_stream(
-            f"called {'_'.join([str(node) for node in controller_api.path])} "
-            f"f method: {method_name}"
+            f"called {'_'.join(controller_api.path)} f method: {method_name}"
         )
 
         coro = method()
@@ -126,7 +125,7 @@ def _collect_dev_commands(
 ) -> dict[str, Any]:
     collection: dict[str, Any] = {}
     for controller_api in root_controller_api.walk_api():
-        path = [str(node) for node in controller_api.path]
+        path = controller_api.path
 
         for name, method in controller_api.command_methods.items():
             cmd_name = name.title().replace("_", "")