Skip to content

refactor(controllers): constructor-pass path, drop set_path()#382

Open
gilesknap wants to merge 1 commit into
mainfrom
path-init
Open

refactor(controllers): constructor-pass path, drop set_path()#382
gilesknap wants to merge 1 commit into
mainfrom
path-init

Conversation

@gilesknap
Copy link
Copy Markdown
Contributor

@gilesknap gilesknap commented May 20, 2026

Summary

Implements branch 2d from #373:

  • path becomes a keyword-only constructor parameter on BaseController / Controller / ControllerVector; the launcher passes path=[entry.id] rather than calling set_path after construction.
  • BaseController.set_path() is removed. add_sub_controller(name, sub) now sanity-asserts sub.path == parent.path + [name] instead of mutating sub._path. Parents construct subs with the full path baked in (e.g. Sub(path=self.path + [name])).
  • _build_api() reads self._path directly; the parent-side path argument and recursive re-prefix are gone.
  • Demo controllers, doc snippets and every test that previously did controller.set_path([id]) are updated to thread path=[...] through __init__.

Closes #373.

Notes on the open sub-decisions

  • Positional vs keyword-only: path is keyword-only — keeps existing positional call-sites (description, ios, settings, …) intact.
  • Default: path defaults to [] so direct/standalone construction (tests, embedded use) still works.
  • add_sub_controller's residual role: strict sanity-assert (full path match), not just path[-1] == name. This catches the common mistake of forgetting to thread path end-to-end and is what every updated test relies on for diagnostics.

Test plan

  • pytest tests/test_controllers.py tests/test_multi_controller.py tests/test_launch.py tests/test_attributes.py tests/test_methods.py tests/test_attribute_logging.py tests/test_control_system.py tests/test_util.py tests/test_ip_connection.py tests/transports/rest tests/transports/graphQL tests/transports/epics/test_emission.py tests/transports/epics/ca/test_gui.py tests/transports/epics/ca/test_softioc.py tests/transports/epics/ca/test_ca_util.py — all pass locally (one pre-existing flaky thread-cleanup failure in test_scan_exception_sets_disconnected_and_reconnect_resumes, order-dependent, passes when run alone).
  • All doc snippets static04..15 and dynamic.py import and execute cleanly.
  • CI: full suite including p4p / tango / softioc / docs-snippets (need a real env).

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Refactor

    • Controller path initialization now occurs at construction via an optional path parameter instead of calling set_path() after instantiation. The set_path() method has been removed.
    • Controller API building now uses the controller's stored path internally rather than accepting it as a parameter.
  • Documentation

    • Updated all documentation snippets and examples to reflect the new controller path initialization pattern.

Closes #373.

`Controller.path` is now a keyword-only ``__init__`` parameter on
``BaseController`` (default ``[]``) and is threaded through user code
rather than seeded post-construction. The launcher builds each root
with ``cls(options, path=[entry.id])`` (or ``cls(path=[entry.id])``
when no options); a parent constructs its subs with the full path
already baked in (e.g. ``Sub(path=self.path + [name])``).

``BaseController.set_path()`` is removed entirely.
``add_sub_controller(name, sub)`` no longer mutates ``sub._path`` --
it sanity-asserts ``sub.path == parent.path + [name]`` and rejects
the call with a clear error if the caller forgot to thread the path.
``_build_api()`` reads ``self._path`` directly, eliminating the
parent-side path argument and the recursive re-prefix it required.

Custom ``Controller.__init__`` (root and sub-controller alike) must
now accept ``path`` and forward it to ``super().__init__``. Demo
controllers and doc snippets are updated to the new shape; tests
either construct with ``path=[...]`` from the start or move the id
declaration above the controller construction so sub paths can be
threaded explicitly.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 20, 2026

📝 Walkthrough

Walkthrough

Controllers now accept a path parameter in their constructors instead of using set_path() after instantiation. The set_path() method is removed, and add_sub_controller() validates that sub-controllers have been constructed with correct paths. The _build_api() method uses the controller's stored path instead of accepting it as a parameter.

Changes

Constructor-based path initialization

Layer / File(s) Summary
BaseController path initialization contract
src/fastcs/controllers/base_controller.py
BaseController.__init__ reorders parameters to make path keyword-only, initializes _path from the parameter, removes set_path() entirely, updates add_sub_controller() to validate sub-controller path correctness, and refactors _build_api() to use the stored _path instead of accepting a parameter.
Controller and ControllerVector accept path parameter
src/fastcs/controllers/controller.py, src/fastcs/controllers/controller_vector.py
Controller.__init__ and ControllerVector.__init__ add keyword-only path parameters and forward them to BaseController. Controller.create_api_and_tasks() calls _build_api() without arguments.
Launcher and production demo controllers
src/fastcs/launch.py, src/fastcs/demo/controllers.py
The launcher passes path=[entry.id] directly to controller constructors. Demo controllers accept path and thread it into sub-controller construction with hierarchical paths (self.path + [name]).
Documentation and example snippets
docs/snippets/dynamic.py, docs/snippets/static04.py through docs/snippets/static15.py
All documentation examples construct controllers with path=[...] directly in the constructor instead of calling set_path() afterward. Examples progress from simple to hierarchical controller setups.
Test infrastructure and helper controllers
tests/assertable_controller.py, tests/conftest.py, tests/test_launch.py
AssertableControllerAPI.__init__ no longer accepts a path parameter; it calls _build_api() with no arguments. Test helper controllers accept optional path parameters and forward them to base constructors.
Test suite updates across controllers and transports
tests/test_controllers.py, tests/test_multi_controller.py, tests/example_*.py, tests/benchmarking/controller.py, tests/transports/*
All tests construct controllers with explicit path values at initialization time. Helper functions like _api_with_id() and fixtures initialize controllers with path directly. Function signatures that previously accepted path parameters (e.g., make_fastcs()) are simplified.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

  • DiamondLightSource/fastcs#360: Removed _id / set_id() and refactored controller identification to use path[0] instead of a separate id field; this PR continues that refactoring by removing set_path() and making path a constructor parameter.
  • DiamondLightSource/fastcs#332: Refactored controller API construction and added create_api_and_tasks() / _build_api() methods; this PR aligns with those changes by removing the path parameter from _build_api().

Suggested reviewers

  • coretl

Poem

🐰 Paths now planted at birth, not rearranged in haste,
Controllers know their place, no set_path() to chase,
Sub-controllers inherit from parent's embrace,
One truth, one time, no rootless state to erase. 🎉

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 3.88% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'refactor(controllers): constructor-pass path, drop set_path()' clearly and concisely summarizes the main refactoring: moving path to constructor parameters and removing the set_path() method.
Linked Issues check ✅ Passed The PR comprehensively implements proposal 2d from #373: path is now a keyword-only constructor parameter on BaseController/Controller/ControllerVector; set_path() is removed; add_sub_controller validates path invariants; _build_api() uses self._path directly; all demo controllers, snippets and tests are updated to pass path through init.
Out of Scope Changes check ✅ Passed All changes are directly aligned with the PR's stated objectives: threading path through controller constructors, removing set_path(), updating add_sub_controller semantics, and refactoring _build_api(). No unrelated refactoring or feature additions are present.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch path-init

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 20, 2026

Codecov Report

❌ Patch coverage is 90.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 91.18%. Comparing base (c80a13c) to head (a426cb4).

Files with missing lines Patch % Lines
src/fastcs/launch.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #382      +/-   ##
==========================================
- Coverage   91.20%   91.18%   -0.02%     
==========================================
  Files          72       72              
  Lines        2875     2870       -5     
==========================================
- Hits         2622     2617       -5     
  Misses        253      253              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
docs/snippets/static10.py (1)

76-76: 💤 Low value

Consider using unpacking syntax for list concatenation.

The expression self.path + [name] can be written more idiomatically as [*self.path, name].

♻️ Optional refactor
-            controller = TemperatureRampController(
-                index, self._connection, path=self.path + [name]
-            )
+            controller = TemperatureRampController(
+                index, self._connection, path=[*self.path, name]
+            )
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/snippets/static10.py` at line 76, Replace the list concatenation
expression used when passing the path parameter to the constructor/call
(currently written as self.path + [name]) with Python unpacking to produce the
same list more idiomatically (use [*self.path, name]); update the call that
constructs/passes path (the invocation that includes index, self._connection,
path=...) so it uses [*self.path, name] instead of self.path + [name] to keep
behaviour identical while improving readability.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@docs/snippets/static10.py`:
- Line 76: Replace the list concatenation expression used when passing the path
parameter to the constructor/call (currently written as self.path + [name]) with
Python unpacking to produce the same list more idiomatically (use [*self.path,
name]); update the call that constructs/passes path (the invocation that
includes index, self._connection, path=...) so it uses [*self.path, name]
instead of self.path + [name] to keep behaviour identical while improving
readability.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c9ca75e8-d358-4607-b28c-109bce0905e5

📥 Commits

Reviewing files that changed from the base of the PR and between c80a13c and a426cb4.

📒 Files selected for processing (33)
  • docs/snippets/dynamic.py
  • docs/snippets/static04.py
  • docs/snippets/static05.py
  • docs/snippets/static06.py
  • docs/snippets/static07.py
  • docs/snippets/static08.py
  • docs/snippets/static09.py
  • docs/snippets/static10.py
  • docs/snippets/static11.py
  • docs/snippets/static12.py
  • docs/snippets/static13.py
  • docs/snippets/static14.py
  • docs/snippets/static15.py
  • src/fastcs/controllers/base_controller.py
  • src/fastcs/controllers/controller.py
  • src/fastcs/controllers/controller_vector.py
  • src/fastcs/demo/controllers.py
  • src/fastcs/launch.py
  • tests/assertable_controller.py
  • tests/benchmarking/controller.py
  • tests/conftest.py
  • tests/example_p4p_ioc.py
  • tests/example_softioc.py
  • tests/test_controllers.py
  • tests/test_launch.py
  • tests/test_multi_controller.py
  • tests/transports/epics/ca/test_gui.py
  • tests/transports/epics/ca/test_initial_value.py
  • tests/transports/epics/ca/test_softioc.py
  • tests/transports/epics/pva/test_p4p.py
  • tests/transports/epics/test_emission.py
  • tests/transports/graphQL/test_graphql.py
  • tests/transports/tango/test_dsr.py

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

refactor(controllers): constructor-pass path, drop set_path()

1 participant