DiamondLightSource / DiamondLightSource/fastcs
Reserved names in BaseController pollute driver attribute namespace
- Dominant language
- Python
- Stars
- 6
- Forks
- 8
- Avg merge
- 2d 12h
- Merged PRs (30d)
- 3
Description
`BaseController` exposes a growing number of public names that collide with names a driver author might reasonably want to use for `Attribute`, `Command`, or `Scan` members.
These can be overwritten with no error, breaking internal logic.
## Currently reserved public names
From `BaseController` / `Tracer`:
| Name | Kind |
|---|---|
| `root_attribute` | class attribute (override hook) |
| `description` | class attribute (override hook) |
| `initialise` | async method (override hook) |
| `post_initialise` | method |
| `path` | property |
| `set_path` | method |
| `add_attribute` | method |
| `add_sub_controller` | method |
| `add_command` | method |
| `add_scan` | method |
| `attributes` | property |
| `sub_controllers` | property |
| `command_methods` | property |
| `scan_methods` | property |
| `connected` | property |
| `log_event` | method |
| `enable_tracing` | method |
| `disable_tracing` | method |
| `add_tracing_filter` | method |
| `remove_tracing_filter` | method |
| `clear_tracing_filters` | method |
From `Controller`:
| Name | Kind |
|---|---|
| `connect` | async method (override hook) |
| `reconnect` | async method |
| `disconnect` | async method (override hook) |
Note: `_bind_attrs` already filters out `_`-prefixed names, so `_` is the existing implicit convention separating framework internals from driver-exposed members. The public names above break this.
## Possible Solutions
### Option A: Add a runtime guard in `add_attribute` / `add_command` / `add_scan`
Raise a `ValueError` at registration time if the name clashes with any name already present on `BaseController`:
```python
_RESERVED_NAMES = frozenset(dir(BaseController))
def add_attribute(self, name, attr):
if name in _RESERVED_NAMES:
raise ValueError(f"'{name}' is reserved by FastCS and cannot be used as an attribute name")
...
```
- Pro: catches conflicts early with a clear error, no API change
- Con: doesn't remove the pollution; reserved names grow silently over time
### Option B: Prefix all framework-internal members with `_`
Apply `_` consistently to everything that is framework infrastructure rather than a driver API, e.g.:
- `path` → `_path` (already the backing field)
- `set_path` → `_set_path`
- `add_attribute`, `add_sub_controller`, `add_command`, `add_scan` → `_add_*`
- `attributes`, `sub_controllers`, `command_methods`, `scan_methods` → `_attributes`, etc.
- `post_initialise` → `_post_initialise`
- Pro: clean and consistent; drivers get a large open namespace; aligns with the existing `_bind_attrs` filter
- Con: breaking change for driver code that calls `controller.add_attribute(...)` or accesses `controller.attributes` directly; `Tracer` methods would also need renaming. These are really public methods and may need to be called externally, e.g. by FastCS.
### Option C: Isolate framework state into a nested object
Move all framework-managed state and accessors onto a single attribute, e.g. `self.fastcs`:
```python
controller.fastcs.attributes
controller.fastcs.add_attribute(...)
controller.fastcs.connected
```
- Pro: fully isolated namespace, no reserved names beyond `fastcs`
- Con: most invasive refactor; changes all internal call sites
Contributor guide
Assessment
This issue has not been assessed yet.