DiamondLightSource / DiamondLightSource/fastcs
refactor(controllers): constructor-pass path, drop set_path()
- Dominant language
- Python
- Stars
- 6
- Forks
- 8
- Avg merge
- 2d 12h
- Merged PRs (30d)
- 3
Description
## Proposal (2d)
Make `path` a parameter of `Controller.__init__` and thread it through user code. Remove `set_path()` entirely. The launcher constructs the root with `cls(path=[entry.id], **options)`; a parent constructs its subs with the full path baked in:
```python
class Temp(Controller):
def __init__(self, path: list[str], settings: TempSettings):
super().__init__(path=path)
self.add_sub_controller("ramp_0", Ramp(path=path + ["ramp_0"], ...))
```
`add_sub_controller(name, sub)` no longer mutates `sub._path` — it sanity-asserts `sub.path[-1] == name` (or just trusts the parent).
Result: one concept (`path`), no post-construction setters, no rootless-sub state, no two-phase init, no recursive re-prefix.
## Cost
Any custom `Controller.__init__` (root **and** sub-controllers) must accept `path` and forward to super:
```python
def __init__(self, path: list[str], my_settings: MySettings):
super().__init__(path=path)
```
This is the same ergonomic shape as the rejected branch B (`yaml-list-id-positional`), applied to *every* customised `Controller`, not just roots. We're accepting it here because **`path` is a real Controller concept**, not a YAML-leaked one — user code already touches `self.path + [name]` when adding subs, so threading `path` through `__init__` is paying the cost where the concept naturally lives. That distinguishes it from `id`, which was a YAML label that shouldn't have leaked into user signatures (which is what motivated branch C).
The contrarian read: "2d is just branch B with `path` instead of `id`" — mechanically true. Be honest about that.
## Open sub-decisions
- `path` positional vs keyword-only on `Controller.__init__`. Lean kwonly (matches branch A) to avoid the sub-controller call-site fallout that branch B hit (`description=` had to become a kwarg to dodge the new positional slot).
- Does `path` default to `[]` for direct/standalone construction (tests, embedded use)? Probably yes.
- `add_sub_controller`'s residual role: pure registration + sanity-assert, or drop it and have parents just write `self.sub_controllers[name] = ...`?
## Context: how we got here
Branch C (landed in PR #360 as [`c8adfee7`](https://github.com/DiamondLightSource/fastcs/pull/360/commits/c8adfee7)) drops `_id` / `set_id` / `Controller.id`. The launcher does `c = cls(options); c.set_path([entry.id])`. The single production reader of `.id` (`control_system.py:26`, IPython context-key + dup-key validation) reads `path[0]` instead. YAML `id:` becomes purely a label for the root path segment.
Branch C deliberately leaves one wrinkle alone: when a root Controller adds subs in its `__init__`, those subs' `_path` is set to `[name]` (rootless), because the launcher hasn't seeded the root's `_path` yet. Today's `_build_api` papers over this by recursing with `path + [name]` from the root's seeded path, so emitted `ControllerAPI.path`s come out correct — but `sub_controller._path` itself remains rootless. That latent oddity exists on `multiple-controllers` today and has never bitten anyone; bundling its fix into branch C would obscure whether the id-removal itself was the right call. Hence this issue.
## Alternatives considered for fixing the rootless-sub state
- **2a — Two-phase Controller lifecycle.** Split sub-registration out of `__init__` into a framework-called `register_subs()` hook that runs after `set_path`. Forces every existing controller to migrate `add_sub_controller` calls out of `__init__`. Rejected: high churn for a problem users don't currently feel.
- **2b — Declarative sub-controllers.** Class-level descriptors / spec returned from `__init__`, framework instantiates with full path baked in. Rejected: framework machinery, awkward for runtime-determined subs (e.g. count from settings).
- **2c — Recursive re-prefix on `set_path`.** Make `set_path` walk descendants and re-prefix their `_path`s. Local change, invisible to users, preserves current authoring style. Plausible but keeps a post-construction setter that 2d eliminates entirely.
- **2d (this proposal).** Constructor-pass `path`. Drops the setter; subs constructed under a populated `self.path` are correct first time; no recursion needed.
2d wins because it's the only option that removes the post-construction setter and the rootless-sub state simultaneously. The cost (user `__init__` boilerplate for `path`) is the price.
Contributor guide
Assessment
This issue has not been assessed yet.