DiamondLightSource / DiamondLightSource/fastcs

Reserved names in BaseController pollute driver attribute namespace

Open
#331 1 comment 0 reactions 0 assignees View on GitHub
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.